diff --git a/internal/action/diff.go b/internal/action/diff.go index 1bb9d3a52..4580b3733 100644 --- a/internal/action/diff.go +++ b/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 +} diff --git a/internal/action/diff_test.go b/internal/action/diff_test.go index d9a7caf75..5e8eaf8de 100644 --- a/internal/action/diff_test.go +++ b/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) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 575ebd0a6..bfc896350 100644 --- a/internal/controller/helmrelease_controller.go +++ b/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, diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index 84fea9b59..4326a31ca 100644 --- a/internal/reconcile/atomic_release.go +++ b/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) diff --git a/internal/reconcile/atomic_release_test.go b/internal/reconcile/atomic_release_test.go index 544cf8dac..75eaf2a8f 100644 --- a/internal/reconcile/atomic_release_test.go +++ b/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)) diff --git a/internal/reconcile/state.go b/internal/reconcile/state.go index 183731442..8a6c8b21f 100644 --- a/internal/reconcile/state.go +++ b/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 { diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index f37347dec..3262067b8 100644 --- a/internal/reconcile/state_test.go +++ b/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) diff --git a/main.go b/main.go index 1f3e3ffbf..31b245f70 100644 --- a/main.go +++ b/main.go @@ -52,6 +52,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1" v2 "github.com/fluxcd/helm-controller/api/v2" + intdigest "github.com/fluxcd/helm-controller/internal/digest" // +kubebuilder:scaffold:imports @@ -105,6 +106,7 @@ func main() { oomWatchMaxMemoryPath string oomWatchCurrentMemoryPath string snapshotDigestAlgo string + disallowedFieldManagers []string tokenCacheOptions cache.TokenFlags defaultKubeConfigServiceAccount string ) @@ -137,6 +139,8 @@ func main() { "The path to the cgroup current memory usage file. Requires feature gate 'OOMWatch' to be enabled. If not set, the path will be automatically detected.") flag.StringVar(&snapshotDigestAlgo, "snapshot-digest-algo", intdigest.Canonical.String(), "The algorithm to use to calculate the digest of Helm release storage snapshots.") + flag.StringArrayVar(&disallowedFieldManagers, "override-manager", []string{}, + "List of field managers to override during drift detection.") clientOptions.BindFlags(flag.CommandLine) logOptions.BindFlags(flag.CommandLine) @@ -354,6 +358,7 @@ func main() { DependencyRequeueInterval: requeueDependency, ArtifactFetchRetries: httpRetry, AllowExternalArtifact: allowExternalArtifact, + DisallowedFieldManagers: disallowedFieldManagers, }).SetupWithManager(ctx, mgr, controller.HelmReleaseReconcilerOptions{ RateLimiter: helper.GetRateLimiter(rateLimiterOptions), WatchConfigs: watchConfigs,