From ccd8f88282fcec04e4b54d93ba592b44ec9f89a0 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 28 Nov 2023 16:13:47 +0100 Subject: [PATCH 1/2] Correct cluster drift using patches This changes the cluster drift correction behavior from performing a Helm upgrade to performing create and patch API requests based on the JSON Patch data. Doing this is much lighter than performing a full release cycle, and deals with the issue of Helm being unable to restore state of Custom Resources without the `--force` flag being set. Which has unwanted side-effects like forcing objects through a deletion/creation cycle. After a drift correction attempt a Kubernetes Event is emitted, which contains a summary of the created and patched resources, and a collection of any (potential) errors. As the goal is to restore state as best as we can, the drift correction will be re-attempted until all resources have been restored to the desired state. Signed-off-by: Hidde Beydals --- go.mod | 4 +- go.sum | 4 +- internal/action/diff.go | 127 ++++- internal/action/diff_test.go | 524 +++++++++++++----- internal/diff/summarize.go | 40 +- internal/diff/summarize_test.go | 100 +++- internal/reconcile/atomic_release.go | 13 +- internal/reconcile/atomic_release_test.go | 52 +- internal/reconcile/correct_cluster_drift.go | 117 ++++ .../reconcile/correct_cluster_drift_test.go | 314 +++++++++++ internal/reconcile/reconcile.go | 3 + internal/reconcile/release_test.go | 18 - internal/reconcile/state_test.go | 32 +- 13 files changed, 1127 insertions(+), 221 deletions(-) create mode 100644 internal/reconcile/correct_cluster_drift.go create mode 100644 internal/reconcile/correct_cluster_drift_test.go diff --git a/go.mod b/go.mod index 1d7dddf98..e80edb20d 100644 --- a/go.mod +++ b/go.mod @@ -15,13 +15,14 @@ replace ( ) require ( + github.com/fluxcd/cli-utils v0.36.0-flux.1 github.com/fluxcd/helm-controller/api v0.36.2 github.com/fluxcd/pkg/apis/acl v0.1.0 github.com/fluxcd/pkg/apis/event v0.6.0 github.com/fluxcd/pkg/apis/kustomize v1.2.0 github.com/fluxcd/pkg/apis/meta v1.2.0 github.com/fluxcd/pkg/runtime v0.43.0 - github.com/fluxcd/pkg/ssa v0.34.0 + github.com/fluxcd/pkg/ssa v0.35.0 github.com/fluxcd/pkg/testserver v0.5.0 github.com/fluxcd/source-controller/api v1.1.2 github.com/go-logr/logr v1.3.0 @@ -77,7 +78,6 @@ require ( github.com/evanphx/json-patch/v5 v5.7.0 // indirect github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect github.com/fatih/color v1.13.0 // indirect - github.com/fluxcd/cli-utils v0.36.0-flux.1 // indirect github.com/fsnotify/fsnotify v1.6.0 // indirect github.com/go-errors/errors v1.4.2 // indirect github.com/go-gorp/gorp/v3 v3.1.0 // indirect diff --git a/go.sum b/go.sum index 5604a3a98..658dff04a 100644 --- a/go.sum +++ b/go.sum @@ -100,8 +100,8 @@ github.com/fluxcd/pkg/apis/meta v1.2.0 h1:O766PzGAdMdQKybSflGL8oV0+GgCNIkdsxfalR github.com/fluxcd/pkg/apis/meta v1.2.0/go.mod h1:fU/Az9AoVyIxC0oI4ihG0NVMNnvrcCzdEym3wxjIQsc= github.com/fluxcd/pkg/runtime v0.43.0 h1:dU4cWct5VTpddGzJUU80zxNl80jbbVEN5Y5rbt4YUnw= github.com/fluxcd/pkg/runtime v0.43.0/go.mod h1:RuqJ9VEXELjzgurK2+UXBBgVN1vS0hZ7CYVG2xBAEVM= -github.com/fluxcd/pkg/ssa v0.34.0 h1:hpMo0D7G3faieRYH39e9YD8Jl+aC2hTgUep8ojG5+LE= -github.com/fluxcd/pkg/ssa v0.34.0/go.mod h1:rhVh0EtYVUOznKXlz6E7JOSgdc8xWbIwA4L5HVtJRLA= +github.com/fluxcd/pkg/ssa v0.35.0 h1:8T3WY4P9SQWApa2hq1rU1u2WE8oqP3MMTsAiEWwhmfo= +github.com/fluxcd/pkg/ssa v0.35.0/go.mod h1:rhVh0EtYVUOznKXlz6E7JOSgdc8xWbIwA4L5HVtJRLA= github.com/fluxcd/pkg/testserver v0.5.0 h1:n/Iskk0tXNt2AgIgjz9qeFK/VhEXGfqeazABXZmO2Es= github.com/fluxcd/pkg/testserver v0.5.0/go.mod h1:/p4st6d0uPLy8wXydeF/kDJgxUYO9u2NqySuXb9S+Fo= github.com/fluxcd/source-controller/api v1.1.2 h1:FfKDKVWnopo+Q2pOAxgHEjrtr4MP41L8aapR4mqBhBk= diff --git a/internal/action/diff.go b/internal/action/diff.go index f20dac538..e0b835a25 100644 --- a/internal/action/diff.go +++ b/internal/action/diff.go @@ -18,20 +18,27 @@ package action import ( "context" + "encoding/json" + "errors" "fmt" + "sort" "strings" helmaction "helm.sh/helm/v3/pkg/action" helmrelease "helm.sh/helm/v3/pkg/release" - "k8s.io/apimachinery/pkg/util/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + apierrutil "k8s.io/apimachinery/pkg/util/errors" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "github.com/fluxcd/cli-utils/pkg/object" "github.com/fluxcd/pkg/ssa" "github.com/fluxcd/pkg/ssa/jsondiff" v2 "github.com/fluxcd/helm-controller/api/v2beta2" + "github.com/fluxcd/helm-controller/internal/diff" ) // Diff returns a jsondiff.DiffSet of the changes between the state of the @@ -86,7 +93,6 @@ func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmreleas diffOpts := []jsondiff.ListOption{ jsondiff.FieldOwner(fieldOwner), jsondiff.ExclusionSelector{v2.DriftDetectionMetadataKey: v2.DriftDetectionDisabledValue}, - jsondiff.MaskSecrets(true), jsondiff.Rationalize(true), jsondiff.Graceful(true), } @@ -119,5 +125,120 @@ func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmreleas if err != nil { errs = append(errs, err) } - return set, errors.Reduce(errors.Flatten(errors.NewAggregate(errs))) + return set, apierrutil.Reduce(apierrutil.Flatten(apierrutil.NewAggregate(errs))) +} + +// ApplyDiff applies the changes described in the provided jsondiff.DiffSet to +// the Kubernetes cluster. +func ApplyDiff(ctx context.Context, config *helmaction.Configuration, diffSet jsondiff.DiffSet, fieldOwner string) (*ssa.ChangeSet, error) { + cfg, err := config.RESTClientGetter.ToRESTConfig() + if err != nil { + return nil, err + } + c, err := client.New(cfg, client.Options{}) + if err != nil { + return nil, err + } + + var toCreate, toPatch sortableDiffs + for _, d := range diffSet { + switch d.Type { + case jsondiff.DiffTypeCreate: + toCreate = append(toCreate, d) + case jsondiff.DiffTypeUpdate: + toPatch = append(toPatch, d) + } + } + + var ( + changeSet = ssa.NewChangeSet() + errs []error + ) + + sort.Sort(toCreate) + for _, d := range toCreate { + obj := d.DesiredObject.DeepCopyObject().(client.Object) + if err := c.Create(ctx, obj, client.FieldOwner(fieldOwner)); err != nil { + errs = append(errs, fmt.Errorf("%s creation failure: %w", diff.ResourceName(obj), err)) + continue + } + changeSet.Add(objectToChangeSetEntry(obj, ssa.CreatedAction)) + } + + sort.Sort(toPatch) + for _, d := range toPatch { + data, err := json.Marshal(d.Patch) + if err != nil { + errs = append(errs, fmt.Errorf("%s patch failure: %w", diff.ResourceName(d.DesiredObject), err)) + continue + } + + obj := d.DesiredObject.DeepCopyObject().(client.Object) + patch := client.RawPatch(types.JSONPatchType, data) + if err := c.Patch(ctx, obj, patch, client.FieldOwner(fieldOwner)); err != nil { + if obj.GetObjectKind().GroupVersionKind().Kind == "Secret" { + err = maskSensitiveErrData(err) + } + errs = append(errs, fmt.Errorf("%s patch failure: %w", diff.ResourceName(obj), err)) + continue + } + changeSet.Add(objectToChangeSetEntry(obj, ssa.ConfiguredAction)) + } + + return changeSet, apierrutil.NewAggregate(errs) +} + +// objectToChangeSetEntry returns a ssa.ChangeSetEntry for the given object and +// action. +func objectToChangeSetEntry(obj client.Object, action ssa.Action) ssa.ChangeSetEntry { + return ssa.ChangeSetEntry{ + ObjMetadata: object.ObjMetadata{ + GroupKind: obj.GetObjectKind().GroupVersionKind().GroupKind(), + Name: obj.GetName(), + Namespace: obj.GetNamespace(), + }, + GroupVersion: obj.GetObjectKind().GroupVersionKind().Version, + Subject: diff.ResourceName(obj), + Action: action, + } +} + +// maskSensitiveErrData masks potentially sensitive data from the error message +// returned by the Kubernetes API server. +// This avoids leaking any sensitive data in logs or other output when a patch +// operation fails. +func maskSensitiveErrData(err error) error { + if apierrors.IsInvalid(err) { + // The last part of the error message is the reason for the error. + if i := strings.LastIndex(err.Error(), `:`); i != -1 { + err = errors.New(strings.TrimSpace(err.Error()[i+1:])) + } + } + return err +} + +// sortableDiffs is a sortable slice of jsondiff.Diffs. +type sortableDiffs []*jsondiff.Diff + +// Len returns the length of the slice. +func (s sortableDiffs) Len() int { return len(s) } + +// Swap swaps the elements with indexes i and j. +func (s sortableDiffs) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + +// Less returns true if the element with index i should sort before the element +// with index j. +// The elements are sorted by GroupKind, Namespace and Name. +func (s sortableDiffs) Less(i, j int) bool { + iDiff, jDiff := s[i], s[j] + + if !ssa.Equals(iDiff.GroupVersionKind().GroupKind(), jDiff.GroupVersionKind().GroupKind()) { + return ssa.IsLessThan(iDiff.GroupVersionKind().GroupKind(), jDiff.GroupVersionKind().GroupKind()) + } + + if iDiff.GetNamespace() != jDiff.GetNamespace() { + return iDiff.GetNamespace() < jDiff.GetNamespace() + } + + return iDiff.GetName() < jDiff.GetName() } diff --git a/internal/action/diff_test.go b/internal/action/diff_test.go index e3d71e3cb..8face3197 100644 --- a/internal/action/diff_test.go +++ b/internal/action/diff_test.go @@ -18,6 +18,7 @@ package action import ( "context" + "encoding/base64" "fmt" "strings" "testing" @@ -25,13 +26,16 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + . "github.com/onsi/gomega" extjsondiff "github.com/wI2L/jsondiff" helmaction "helm.sh/helm/v3/pkg/action" helmrelease "helm.sh/helm/v3/pkg/release" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + apierrutil "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/rand" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -46,8 +50,8 @@ import ( func TestDiff(t *testing.T) { // Normally, we would create e.g. a `suite_test.go` file with a `TestMain` - // function. But because this is the only test in this package which needs - // a test cluster, we create it here instead. + // function. As this is one of the few tests in this package which needs a + // test cluster, we create it here instead. config, cleanup := newTestCluster(t) t.Cleanup(func() { t.Log("Stopping the test environment") @@ -72,7 +76,7 @@ func TestDiff(t *testing.T) { manifest string ignoreRules []v2.IgnoreRule mutateCluster func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) - want func(namepace string) jsondiff.DiffSet + want func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet wantErr bool }{ { @@ -118,16 +122,12 @@ data: } return clusterObjs, nil }, - want: func(namespace string) jsondiff.DiffSet { + want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { return jsondiff.DiffSet{ { - Type: jsondiff.DiffTypeUpdate, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "changed", + Type: jsondiff.DiffTypeUpdate, + DesiredObject: namespacedUnstructured(desired[0], namespace), + ClusterObject: cluster[0], Patch: extjsondiff.Patch{ { Type: extjsondiff.OperationReplace, @@ -138,22 +138,13 @@ data: }, }, { - Type: jsondiff.DiffTypeCreate, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "deleted", + Type: jsondiff.DiffTypeCreate, + DesiredObject: namespacedUnstructured(desired[1], namespace), }, { - Type: jsondiff.DiffTypeNone, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "unchanged", + Type: jsondiff.DiffTypeNone, + DesiredObject: namespacedUnstructured(desired[2], namespace), + ClusterObject: cluster[1], }, } }, @@ -185,16 +176,11 @@ data: } return clusterObjs, nil }, - want: func(namespace string) jsondiff.DiffSet { + want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { return jsondiff.DiffSet{ { - Type: jsondiff.DiffTypeExclude, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "disabled", + Type: jsondiff.DiffTypeExclude, + DesiredObject: namespacedUnstructured(desired[0], namespace), }, } }, @@ -222,16 +208,11 @@ data: } return clusterObjs, nil }, - want: func(namespace string) jsondiff.DiffSet { + want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { return jsondiff.DiffSet{ { - Type: jsondiff.DiffTypeExclude, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "disabled", + Type: jsondiff.DiffTypeExclude, + DesiredObject: namespacedUnstructured(desired[0], namespace), }, } }, @@ -299,51 +280,34 @@ data: {Target: &kustomize.Selector{Name: "partially-ignored"}, Paths: []string{"/data/key"}}, {Paths: []string{"/data/globalKey"}}, }, - want: func(namespace string) jsondiff.DiffSet { + want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { return jsondiff.DiffSet{ { - Type: jsondiff.DiffTypeExclude, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "fully-ignored", + Type: jsondiff.DiffTypeExclude, + DesiredObject: namespacedUnstructured(desired[0], namespace), }, { - Type: jsondiff.DiffTypeUpdate, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "Secret", - }, - Namespace: namespace, - Name: "partially-ignored", + Type: jsondiff.DiffTypeUpdate, + DesiredObject: namespacedUnstructured(desired[1], namespace), + ClusterObject: cluster[1], Patch: extjsondiff.Patch{ { Type: extjsondiff.OperationReplace, Path: "/data/otherKey", - OldValue: "*** (before)", - Value: "*** (after)", + OldValue: base64.StdEncoding.EncodeToString([]byte("changed")), + Value: base64.StdEncoding.EncodeToString([]byte("otherValue")), }, }, }, { - Type: jsondiff.DiffTypeNone, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "Secret", - }, - Namespace: namespace, - Name: "globally-ignored", + Type: jsondiff.DiffTypeNone, + DesiredObject: namespacedUnstructured(desired[2], namespace), + ClusterObject: cluster[2], }, { - Type: jsondiff.DiffTypeUpdate, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "not-ignored", + Type: jsondiff.DiffTypeUpdate, + DesiredObject: namespacedUnstructured(desired[3], namespace), + ClusterObject: cluster[3], Patch: extjsondiff.Patch{ { Type: extjsondiff.OperationReplace, @@ -394,68 +358,17 @@ data: } return clusterObjs, nil }, - want: func(namespace string) jsondiff.DiffSet { + want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { return jsondiff.DiffSet{ { - Type: jsondiff.DiffTypeNone, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: namespace, - Name: "without-namespace", + Type: jsondiff.DiffTypeNone, + DesiredObject: namespacedUnstructured(desired[0], namespace), + ClusterObject: cluster[1], }, { - Type: jsondiff.DiffTypeNone, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "ConfigMap", - }, - Namespace: "diff-fixed-ns", - Name: "with-namespace", - }, - } - }, - }, - { - name: "masks Secret data", - manifest: `--- -apiVersion: v1 -kind: Secret -metadata: - name: secret -stringData: - 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) - if err := unstructured.SetNestedField(obj.Object, "changed", "stringData", "key"); err != nil { - return nil, fmt.Errorf("failed to set nested field: %w", err) - } - clusterObjs = append(clusterObjs, obj) - } - return clusterObjs, nil - }, - want: func(namespace string) jsondiff.DiffSet { - return jsondiff.DiffSet{ - { - Type: jsondiff.DiffTypeUpdate, - GroupVersionKind: schema.GroupVersionKind{ - Version: "v1", - Kind: "Secret", - }, - Namespace: namespace, - Name: "secret", - Patch: extjsondiff.Patch{ - { - Type: extjsondiff.OperationReplace, - Path: "/data/key", - OldValue: "*** (before)", - Value: "*** (after)", - }, - }, + Type: jsondiff.DiffTypeNone, + DesiredObject: namespacedUnstructured(desired[1], desired[1].GetNamespace()), + ClusterObject: cluster[2], }, } }, @@ -515,7 +428,7 @@ stringData: var want jsondiff.DiffSet if tt.want != nil { - want = tt.want(ns.Name) + want = tt.want(ns.Name, objs, clusterObjs) } if diff := cmp.Diff(want, got, cmpopts.IgnoreUnexported(extjsondiff.Operation{})); diff != "" { t.Errorf("Diff() mismatch (-want +got):\n%s", diff) @@ -524,6 +437,341 @@ stringData: } } +func TestApplyDiff(t *testing.T) { + // Normally, we would create e.g. a `suite_test.go` file with a `TestMain` + // function. As this is one of the few tests in this package which needs a + // test cluster, we create it here instead. + config, cleanup := newTestCluster(t) + t.Cleanup(func() { + t.Log("Stopping the test environment") + if err := cleanup(); err != nil { + t.Logf("Failed to stop the test environment: %v", err) + } + }) + + // Construct a REST client getter for Helm's action configuration. + getter := kube.NewMemoryRESTClientGetter(config) + + // Construct a client for to be able to mutate the cluster. + c, err := client.New(config, client.Options{}) + if err != nil { + t.Fatalf("Failed to create client for test environment: %v", err) + } + + const testOwner = "helm-controller" + + tests := []struct { + name string + diffSet func(namespace string) jsondiff.DiffSet + expect func(g *GomegaWithT, namespace string, got *ssa.ChangeSet, err error) + }{ + { + name: "creates and updates resources", + diffSet: func(namespace string) jsondiff.DiffSet { + return jsondiff.DiffSet{ + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "namespace": namespace, + }, + "stringData": map[string]interface{}{ + "key": "value", + }, + }, + }, + }, + { + Type: jsondiff.DiffTypeUpdate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + "namespace": namespace, + }, + "data": map[string]interface{}{ + "key": "value", + }, + }, + }, + ClusterObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + "namespace": namespace, + }, + "data": map[string]interface{}{ + "key": "changed", + }, + }, + }, + Patch: extjsondiff.Patch{ + { + Type: extjsondiff.OperationReplace, + Path: "/data/key", + Value: "value", + }, + }, + }, + } + }, + expect: func(g *GomegaWithT, namespace string, got *ssa.ChangeSet, err error) { + g.THelper() + + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(got).NotTo(BeNil()) + g.Expect(got.Entries).To(HaveLen(2)) + + g.Expect(got.Entries[0].Subject).To(Equal("Secret/" + namespace + "/test-secret")) + g.Expect(got.Entries[0].Action).To(Equal(ssa.CreatedAction)) + g.Expect(c.Get(context.TODO(), types.NamespacedName{ + Namespace: namespace, + Name: "test-secret", + }, &corev1.Secret{})).To(Succeed()) + + g.Expect(got.Entries[1].Subject).To(Equal("ConfigMap/" + namespace + "/test-cm")) + g.Expect(got.Entries[1].Action).To(Equal(ssa.ConfiguredAction)) + cm := &corev1.ConfigMap{} + g.Expect(c.Get(context.TODO(), types.NamespacedName{ + Namespace: namespace, + Name: "test-cm", + }, cm)).To(Succeed()) + g.Expect(cm.Data).To(HaveKeyWithValue("key", "value")) + }, + }, + { + name: "continues on error", + diffSet: func(namespace string) jsondiff.DiffSet { + return jsondiff.DiffSet{ + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "invalid-test-secret", + "namespace": namespace, + }, + "data": map[string]interface{}{ + // Illegal base64 encoded data. + "key": "secret value", + }, + }, + }, + }, + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + "namespace": namespace, + }, + }, + }, + }, + { + Type: jsondiff.DiffTypeUpdate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "invalid-test-secret-update", + "namespace": namespace, + }, + "data": map[string]interface{}{ + // Illegal base64 encoded data. + "key": "secret value2", + }, + }, + }, + ClusterObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "invalid-test-secret-update", + "namespace": namespace, + }, + "stringData": map[string]interface{}{ + "key": "value", + }, + }, + }, + Patch: extjsondiff.Patch{ + { + Type: extjsondiff.OperationReplace, + Path: "/data/key", + // Illegal base64 encoded data. + Value: "value", + }, + }, + }, + { + Type: jsondiff.DiffTypeUpdate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm-2", + "namespace": namespace, + }, + "data": map[string]interface{}{ + "key": "value", + }, + }, + }, + ClusterObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm-2", + "namespace": namespace, + }, + "data": map[string]interface{}{ + "key": "changed", + }, + }, + }, + Patch: extjsondiff.Patch{ + { + Type: extjsondiff.OperationReplace, + Path: "/data/key", + Value: "value", + }, + }, + }, + } + }, + expect: func(g *GomegaWithT, namespace string, got *ssa.ChangeSet, err error) { + g.THelper() + + g.Expect(err).To(HaveOccurred()) + g.Expect(err.(apierrutil.Aggregate).Errors()).To(HaveLen(2)) + g.Expect(err.Error()).To(ContainSubstring("invalid-test-secret creation failure")) + g.Expect(err.Error()).To(ContainSubstring("invalid-test-secret-update patch failure")) + + // Verify that the error message does not contain the secret data. + g.Expect(err.Error()).ToNot(ContainSubstring("secret value")) + g.Expect(err.Error()).ToNot(ContainSubstring("secret value2")) + + g.Expect(got).NotTo(BeNil()) + g.Expect(got.Entries).To(HaveLen(2)) + + g.Expect(got.Entries[0].Subject).To(Equal("ConfigMap/" + namespace + "/test-cm")) + g.Expect(got.Entries[0].Action).To(Equal(ssa.CreatedAction)) + g.Expect(c.Get(context.TODO(), types.NamespacedName{ + Namespace: namespace, + Name: "test-cm", + }, &corev1.ConfigMap{})).To(Succeed()) + + g.Expect(got.Entries[1].Subject).To(Equal("ConfigMap/" + namespace + "/test-cm-2")) + g.Expect(got.Entries[1].Action).To(Equal(ssa.ConfiguredAction)) + + cm2 := &corev1.ConfigMap{} + g.Expect(c.Get(context.TODO(), types.NamespacedName{ + Namespace: namespace, + Name: "test-cm-2", + }, cm2)).To(Succeed()) + g.Expect(cm2.Data).To(HaveKeyWithValue("key", "value")) + }, + }, + { + name: "creates namespace before dependent resources", + diffSet: func(namespace string) jsondiff.DiffSet { + otherNS := generateName("test-ns") + + return jsondiff.DiffSet{ + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + "namespace": otherNS, + }, + }, + }, + }, + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Namespace", + "metadata": map[string]interface{}{ + "name": otherNS, + }, + }, + }, + }, + } + }, + expect: func(g *GomegaWithT, namespace string, got *ssa.ChangeSet, err error) { + g.THelper() + + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(got).NotTo(BeNil()) + g.Expect(got.Entries).To(HaveLen(2)) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + t.Cleanup(cancel) + + ns, err := generateNamespace(ctx, c, "diff-action") + if err != nil { + t.Fatalf("Failed to generate namespace: %v", err) + } + t.Cleanup(func() { + if err := c.Delete(context.Background(), ns); client.IgnoreNotFound(err) != nil { + t.Logf("Failed to delete generated namespace: %v", err) + } + }) + + diff := tt.diffSet(ns.Name) + + for _, d := range diff { + if d.ClusterObject != nil { + if err := c.Create(ctx, d.ClusterObject, client.FieldOwner(testOwner)); err != nil { + t.Fatalf("Failed to create cluster object: %v", err) + } + t.Cleanup(func() { + if err := c.Delete(ctx, d.ClusterObject); client.IgnoreNotFound(err) != nil { + t.Logf("Failed to delete cluster object: %v", err) + } + }) + } + } + + got, err := ApplyDiff(context.Background(), &helmaction.Configuration{RESTClientGetter: getter}, diff, testOwner) + tt.expect(g, ns.Name, got, err) + }) + } +} + // newTestCluster creates a new test cluster and returns a rest.Config and a // function to stop the test cluster. func newTestCluster(t *testing.T) (*rest.Config, func() error) { @@ -552,3 +800,15 @@ func generateNamespace(ctx context.Context, c client.Client, generateName string } return ns, nil } + +// generateName generates a name with the given name and a random suffix. +func generateName(name string) string { + return fmt.Sprintf("%s-%s", name, rand.String(5)) +} + +func namespacedUnstructured(obj *unstructured.Unstructured, namespace string) *unstructured.Unstructured { + obj = obj.DeepCopy() + obj.SetNamespace(namespace) + _ = ssa.NormalizeUnstructured(obj) + return obj +} diff --git a/internal/diff/summarize.go b/internal/diff/summarize.go index c553120c7..9b1d80d9b 100644 --- a/internal/diff/summarize.go +++ b/internal/diff/summarize.go @@ -21,6 +21,7 @@ import ( "strings" extjsondiff "github.com/wI2L/jsondiff" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/fluxcd/pkg/ssa/jsondiff" ) @@ -49,10 +50,10 @@ var DefaultDiffTypes = []jsondiff.DiffType{ // // For example: // -// Deployment/default/hello-world: changed (1 added, 1 changed, 1 removed) -// Deployment/default/hello-world2: removed -// Deployment/default/hello-world3: excluded -// Deployment/default/hello-world4: unchanged +// Deployment/default/hello-world changed (1 added, 1 changed, 1 removed) +// Deployment/default/hello-world2 removed +// Deployment/default/hello-world3 excluded +// Deployment/default/hello-world4 unchanged func SummarizeDiffSet(set jsondiff.DiffSet, include ...jsondiff.DiffType) string { if include == nil { include = DefaultDiffTypes @@ -66,16 +67,16 @@ func SummarizeDiffSet(set jsondiff.DiffSet, include ...jsondiff.DiffType) string switch diff.Type { case jsondiff.DiffTypeNone: - writeResourceName(diff, &summary) + writeResourceName(diff.DesiredObject, &summary) summary.WriteString(" unchanged\n") case jsondiff.DiffTypeCreate: - writeResourceName(diff, &summary) + writeResourceName(diff.DesiredObject, &summary) summary.WriteString(" removed\n") case jsondiff.DiffTypeExclude: - writeResourceName(diff, &summary) + writeResourceName(diff.DesiredObject, &summary) summary.WriteString(" excluded\n") case jsondiff.DiffTypeUpdate: - writeResourceName(diff, &summary) + writeResourceName(diff.DesiredObject, &summary) added, changed, removed := summarizeUpdate(diff) summary.WriteString(fmt.Sprintf(" changed (%d additions, %d changes, %d removals)\n", added, changed, removed)) } @@ -127,14 +128,25 @@ func SummarizeDiffSetBrief(set jsondiff.DiffSet, include ...jsondiff.DiffType) s return strings.TrimSuffix(summary.String(), ", ") } +// ResourceName returns the resource name in the format `kind/namespace/name`. +func ResourceName(obj client.Object) string { + var summary strings.Builder + writeResourceName(obj, &summary) + return summary.String() +} + +const resourceSeparator = "/" + // writeResourceName writes the resource name in the format // `kind/namespace/name` to the given strings.Builder. -func writeResourceName(diff *jsondiff.Diff, summary *strings.Builder) { - summary.WriteString(diff.GroupVersionKind.Kind) - summary.WriteString("/") - summary.WriteString(diff.Namespace) - summary.WriteString("/") - summary.WriteString(diff.Name) +func writeResourceName(obj client.Object, summary *strings.Builder) { + summary.WriteString(obj.GetObjectKind().GroupVersionKind().Kind) + summary.WriteString(resourceSeparator) + if ns := obj.GetNamespace(); ns != "" { + summary.WriteString(ns) + summary.WriteString(resourceSeparator) + } + summary.WriteString(obj.GetName()) } // SummarizeUpdate returns the number of added, changed and removed fields diff --git a/internal/diff/summarize_test.go b/internal/diff/summarize_test.go index 8b9024379..b67206e8a 100644 --- a/internal/diff/summarize_test.go +++ b/internal/diff/summarize_test.go @@ -20,7 +20,8 @@ import ( "testing" extjsondiff "github.com/wI2L/jsondiff" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/fluxcd/pkg/ssa/jsondiff" ) @@ -28,36 +29,52 @@ import ( func TestSummarizeDiffSet(t *testing.T) { diffSet := jsondiff.DiffSet{ &jsondiff.Diff{ - GroupVersionKind: schema.GroupVersionKind{ - Kind: "ConfigMap", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "config", + "namespace": "namespace-1", + }, + }, }, - Namespace: "namespace-1", - Name: "config", - Type: jsondiff.DiffTypeNone, + Type: jsondiff.DiffTypeNone, }, &jsondiff.Diff{ - GroupVersionKind: schema.GroupVersionKind{ - Kind: "Secret", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "naughty", + "namespace": "namespace-x", + }, + }, }, - Namespace: "namespace-x", - Name: "naughty", - Type: jsondiff.DiffTypeCreate, + Type: jsondiff.DiffTypeCreate, }, &jsondiff.Diff{ - GroupVersionKind: schema.GroupVersionKind{ - Kind: "StatefulSet", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "StatefulSet", + "metadata": map[string]interface{}{ + "name": "hello-world", + "namespace": "default", + }, + }, }, - Namespace: "default", - Name: "hello-world", - Type: jsondiff.DiffTypeExclude, + Type: jsondiff.DiffTypeExclude, }, &jsondiff.Diff{ - GroupVersionKind: schema.GroupVersionKind{ - Kind: "Deployment", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "touched-me", + "namespace": "tenant-y", + }, + }, }, - Namespace: "tenant-y", - Name: "touched-me", - Type: jsondiff.DiffTypeUpdate, + Type: jsondiff.DiffTypeUpdate, Patch: extjsondiff.Patch{ {Type: extjsondiff.OperationAdd}, {Type: extjsondiff.OperationReplace}, @@ -188,3 +205,44 @@ func TestSummarizeDiffSetBrief(t *testing.T) { }) } } + +func TestResourceName(t *testing.T) { + tests := []struct { + name string + resource client.Object + want string + }{ + { + name: "with namespace", + resource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "touched-me", + "namespace": "tenant-y", + }, + }, + }, + want: "Deployment/tenant-y/touched-me", + }, + { + name: "without namespace", + resource: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "ClusterIssuer", + "metadata": map[string]interface{}{ + "name": "letsencrypt", + }, + }, + }, + want: "ClusterIssuer/letsencrypt", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ResourceName(tt.resource); got != tt.want { + t.Errorf("ResourceName() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index 3949cc0b2..87190f6ad 100644 --- a/internal/reconcile/atomic_release.go +++ b/internal/reconcile/atomic_release.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "helm.sh/helm/v3/pkg/kube" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" @@ -320,8 +321,14 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state case ReleaseStatusDrifted: log.Info(msgWithReason("detected changes in cluster state", diff.SummarizeDiffSetBrief(state.Diff))) for _, change := range state.Diff { - if change.Type == jsondiff.DiffTypeCreate || change.Type == jsondiff.DiffTypeUpdate { - log.V(logger.DebugLevel).Info("observed change in cluster state", "diff", change) + switch change.Type { + case jsondiff.DiffTypeCreate: + log.V(logger.DebugLevel).Info("resource deleted", + "resource", diff.ResourceName(change.DesiredObject)) + case jsondiff.DiffTypeUpdate: + log.V(logger.DebugLevel).Info("resource modified", + "resource", diff.ResourceName(change.DesiredObject), + "patch", jsondiff.MaskSecretPatchData(change.Patch)) } } @@ -331,7 +338,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state ) if req.Object.GetDriftDetection().GetMode() == v2.DriftDetectionEnabled { - return NewUpgrade(r.configFactory, r.eventRecorder), nil + return NewCorrectClusterDrift(r.configFactory, r.eventRecorder, state.Diff, kube.ManagedFieldsManager), nil } return nil, nil diff --git a/internal/reconcile/atomic_release_test.go b/internal/reconcile/atomic_release_test.go index a6cbcde1b..d932fb65f 100644 --- a/internal/reconcile/atomic_release_test.go +++ b/internal/reconcile/atomic_release_test.go @@ -31,7 +31,7 @@ import ( helmdriver "helm.sh/helm/v3/pkg/storage/driver" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -1062,17 +1062,20 @@ func TestAtomicRelease_actionForState(t *testing.T) { want: &Upgrade{}, }, { - name: "drifted release triggers upgrade if enabled", + name: "drifted release triggers correction if enabled", state: ReleaseState{Status: ReleaseStatusDrifted, Diff: jsondiff.DiffSet{ { Type: jsondiff.DiffTypeCreate, - GroupVersionKind: schema.GroupVersionKind{ - Group: "apps", - Kind: "Deployment", - Version: "v1", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "mock", + "namespace": "something", + }, + }, }, - Name: "mock", - Namespace: "something", }, }}, spec: func(spec *v2.HelmReleaseSpec) { @@ -1091,7 +1094,7 @@ func TestAtomicRelease_actionForState(t *testing.T) { }, } }, - want: &Upgrade{}, + want: &CorrectClusterDrift{}, wantEvent: &corev1.Event{ Reason: "DriftDetected", Type: corev1.EventTypeWarning, @@ -1123,13 +1126,32 @@ func TestAtomicRelease_actionForState(t *testing.T) { state: ReleaseState{Status: ReleaseStatusDrifted, Diff: jsondiff.DiffSet{ { Type: jsondiff.DiffTypeUpdate, - GroupVersionKind: schema.GroupVersionKind{ - Group: "apps", - Kind: "Deployment", - Version: "v1", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "mock", + "namespace": "something", + }, + "spec": map[string]interface{}{ + "replicas": 2, + }, + }, + }, + ClusterObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "mock", + "namespace": "something", + }, + "spec": map[string]interface{}{ + "replicas": 1, + }, + }, }, - Name: "mock", - Namespace: "something", Patch: extjsondiff.Patch{ { Type: extjsondiff.OperationReplace, diff --git a/internal/reconcile/correct_cluster_drift.go b/internal/reconcile/correct_cluster_drift.go new file mode 100644 index 000000000..bbd6f7a31 --- /dev/null +++ b/internal/reconcile/correct_cluster_drift.go @@ -0,0 +1,117 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconcile + +import ( + "context" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrutil "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/client-go/tools/record" + + "github.com/fluxcd/pkg/ssa" + "github.com/fluxcd/pkg/ssa/jsondiff" + + v2 "github.com/fluxcd/helm-controller/api/v2beta2" + "github.com/fluxcd/helm-controller/internal/action" +) + +// CorrectClusterDrift is a reconciler that attempts to correct the cluster state +// of a Helm release. It does so by applying the Helm release's desired state +// to the cluster based on a jsondiff.DiffSet. +// +// The reconciler will only attempt to correct the cluster state if the Helm +// release has drift detection enabled and the jsondiff.DiffSet is not empty. +// +// The reconciler will emit a Kubernetes event upon completion indicating +// whether the cluster state was successfully corrected or not. +type CorrectClusterDrift struct { + configFactory *action.ConfigFactory + eventRecorder record.EventRecorder + diff jsondiff.DiffSet + fieldManager string +} + +func NewCorrectClusterDrift(configFactory *action.ConfigFactory, recorder record.EventRecorder, diff jsondiff.DiffSet, fieldManager string) *CorrectClusterDrift { + return &CorrectClusterDrift{ + configFactory: configFactory, + eventRecorder: recorder, + diff: diff, + fieldManager: fieldManager, + } +} + +func (r *CorrectClusterDrift) Reconcile(ctx context.Context, req *Request) error { + if req.Object.GetDriftDetection().GetMode() != v2.DriftDetectionEnabled || len(r.diff) == 0 { + return nil + } + + ctx, cancel := context.WithTimeout(ctx, req.Object.GetTimeout().Duration) + defer cancel() + + changeSet, err := action.ApplyDiff(ctx, r.configFactory.Build(nil), r.diff, r.fieldManager) + r.report(req.Object, changeSet, err) + return nil +} + +func (r *CorrectClusterDrift) report(obj *v2.HelmRelease, changeSet *ssa.ChangeSet, err error) { + cur := obj.Status.History.Latest() + + switch { + case err != nil: + var sb strings.Builder + sb.WriteString("Failed to ") + if changeSet != nil && len(changeSet.Entries) > 0 { + sb.WriteString("partially ") + } + sb.WriteString("correct cluster state of release ") + sb.WriteString(cur.FullReleaseName()) + sb.WriteString(":\n") + if agErr, ok := err.(apierrutil.Aggregate); ok { + for i := range agErr.Errors() { + if i > 0 { + sb.WriteString("\n") + } + sb.WriteString(agErr.Errors()[i].Error()) + } + } else { + sb.WriteString(err.Error()) + } + + if changeSet != nil && len(changeSet.Entries) > 0 { + sb.WriteString("\n\n") + sb.WriteString("Successful corrections:\n") + sb.WriteString(changeSet.String()) + } + + r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest), corev1.EventTypeWarning, + "DriftCorrectionFailed", sb.String()) + case changeSet != nil && len(changeSet.Entries) > 0: + r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest), corev1.EventTypeNormal, + "DriftCorrected", "Cluster state of release %s has been corrected:\n%s", + obj.Status.History.Latest().FullReleaseName(), changeSet.String()) + } +} + +func (r *CorrectClusterDrift) Name() string { + return "correct cluster drift" +} + +func (r *CorrectClusterDrift) Type() ReconcilerType { + return ReconcilerTypeDriftCorrection +} diff --git a/internal/reconcile/correct_cluster_drift_test.go b/internal/reconcile/correct_cluster_drift_test.go new file mode 100644 index 000000000..83c14a071 --- /dev/null +++ b/internal/reconcile/correct_cluster_drift_test.go @@ -0,0 +1,314 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconcile + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/gomega" + extjsondiff "github.com/wI2L/jsondiff" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + apierrutil "k8s.io/apimachinery/pkg/util/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/fluxcd/pkg/ssa" + "github.com/fluxcd/pkg/ssa/jsondiff" + + v2 "github.com/fluxcd/helm-controller/api/v2beta2" + "github.com/fluxcd/helm-controller/internal/action" + "github.com/fluxcd/helm-controller/internal/testutil" +) + +func TestCorrectClusterDrift_Reconcile(t *testing.T) { + mockStatus := v2.HelmReleaseStatus{ + History: v2.Snapshots{ + { + Version: 2, + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + }, + } + + tests := []struct { + name string + obj *v2.HelmRelease + diff func(namespace string) jsondiff.DiffSet + wantEvent bool + }{ + { + name: "corrects cluster drift", + obj: &v2.HelmRelease{ + Spec: v2.HelmReleaseSpec{ + DriftDetection: &v2.DriftDetection{ + Mode: v2.DriftDetectionEnabled, + }, + }, + Status: *mockStatus.DeepCopy(), + }, + diff: func(namespace string) jsondiff.DiffSet { + return jsondiff.DiffSet{ + { + Type: jsondiff.DiffTypeCreate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "secret", + "namespace": namespace, + }, + }, + }, + }, + { + Type: jsondiff.DiffTypeUpdate, + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "configmap", + "namespace": namespace, + }, + "data": map[string]interface{}{ + "key": "value", + }, + }, + }, + ClusterObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "configmap", + "namespace": namespace, + }, + }, + }, + Patch: extjsondiff.Patch{ + { + Type: extjsondiff.OperationAdd, + Path: "/data", + Value: map[string]interface{}{ + "key": "value", + }, + }, + }, + }, + } + }, + wantEvent: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace) + g.Expect(err).NotTo(HaveOccurred()) + t.Cleanup(func() { + _ = testEnv.Delete(context.TODO(), namedNS) + }) + + diff := tt.diff(namedNS.Name) + for _, diff := range diff { + if diff.ClusterObject != nil { + obj := diff.ClusterObject.DeepCopyObject() + g.Expect(testEnv.Create(context.TODO(), obj.(client.Object))).To(Succeed()) + } + } + + getter, err := RESTClientGetterFromManager(testEnv.Manager, namedNS.Name) + g.Expect(err).ToNot(HaveOccurred()) + + cfg, err := action.NewConfigFactory(getter, action.WithStorage(action.DefaultStorageDriver, namedNS.Name)) + g.Expect(err).ToNot(HaveOccurred()) + + recorder := testutil.NewFakeRecorder(10, false) + + r := NewCorrectClusterDrift(cfg, recorder, tt.diff(namedNS.Name), testFieldManager) + g.Expect(r.Reconcile(context.TODO(), &Request{ + Object: tt.obj, + })).ToNot(HaveOccurred()) + + if tt.wantEvent { + g.Expect(recorder.GetEvents()).To(HaveLen(1)) + } else { + g.Expect(recorder.GetEvents()).To(BeEmpty()) + } + }) + } +} + +func TestCorrectClusterDrift_report(t *testing.T) { + mockObj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + { + Version: 3, + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + }, + }, + } + + tests := []struct { + name string + obj *v2.HelmRelease + changeSet *ssa.ChangeSet + err error + wantEvent []corev1.Event + }{ + { + name: "with multiple changes", + obj: mockObj.DeepCopy(), + changeSet: &ssa.ChangeSet{ + Entries: []ssa.ChangeSetEntry{ + { + Subject: "Secret/namespace/name", + Action: ssa.CreatedAction, + }, + { + Subject: "Deployment/namespace/name", + Action: ssa.ConfiguredAction, + }, + }, + }, + wantEvent: []corev1.Event{ + { + Type: corev1.EventTypeNormal, + Reason: "DriftCorrected", + Message: `Cluster state of release mock-ns/mock-release.v3 has been corrected: +Secret/namespace/name created +Deployment/namespace/name configured`, + }, + }, + }, + { + name: "with multiple changes and errors", + obj: mockObj.DeepCopy(), + changeSet: &ssa.ChangeSet{ + Entries: []ssa.ChangeSetEntry{ + { + Subject: "Secret/namespace/name", + Action: ssa.CreatedAction, + }, + { + Subject: "Deployment/namespace/name", + Action: ssa.ConfiguredAction, + }, + { + Subject: "ConfigMap/namespace/name", + Action: ssa.ConfiguredAction, + }, + }, + }, + err: apierrutil.NewAggregate([]error{ + errors.New("error 1"), + errors.New("error 2"), + }), + wantEvent: []corev1.Event{ + { + Type: corev1.EventTypeWarning, + Reason: "DriftCorrectionFailed", + Message: `Failed to partially correct cluster state of release mock-ns/mock-release.v3: +error 1 +error 2 + +Successful corrections: +Secret/namespace/name created +Deployment/namespace/name configured +ConfigMap/namespace/name configured`, + }, + }, + }, + { + name: "with multiple errors", + obj: mockObj.DeepCopy(), + err: apierrutil.NewAggregate([]error{ + errors.New("error 1"), + errors.New("error 2"), + }), + wantEvent: []corev1.Event{ + { + Type: corev1.EventTypeWarning, + Reason: "DriftCorrectionFailed", + Message: `Failed to correct cluster state of release mock-ns/mock-release.v3: +error 1 +error 2`, + }, + }, + }, + { + name: "with single change", + obj: mockObj.DeepCopy(), + changeSet: &ssa.ChangeSet{ + Entries: []ssa.ChangeSetEntry{ + { + Subject: "Secret/namespace/name", + Action: ssa.CreatedAction, + }, + }, + }, + wantEvent: []corev1.Event{ + { + Type: corev1.EventTypeNormal, + Reason: "DriftCorrected", + Message: `Cluster state of release mock-ns/mock-release.v3 has been corrected: +Secret/namespace/name created`, + }, + }, + }, + { + name: "with single error", + obj: mockObj.DeepCopy(), + err: errors.New("error 1"), + wantEvent: []corev1.Event{ + { + Type: corev1.EventTypeWarning, + Reason: "DriftCorrectionFailed", + Message: `Failed to correct cluster state of release mock-ns/mock-release.v3: +error 1`, + }, + }, + }, + { + name: "empty change set", + obj: mockObj.DeepCopy(), + wantEvent: []corev1.Event{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + recorder := testutil.NewFakeRecorder(10, false) + r := &CorrectClusterDrift{ + eventRecorder: recorder, + } + + r.report(tt.obj, tt.changeSet, tt.err) + g.Expect(recorder.GetEvents()).To(ConsistOf(tt.wantEvent)) + }) + } +} diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 2565e57fc..1cd9a0999 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -38,6 +38,9 @@ const ( // release in a stale pending state. It differs from ReconcilerTypeRemediate // in that it does not produce a new Helm release. ReconcilerTypeUnlock ReconcilerType = "unlock" + // ReconcilerTypeDriftCorrection is an ActionReconciler which corrects + // Helm releases which have drifted from the cluster state. + ReconcilerTypeDriftCorrection ReconcilerType = "drift correction" ) // ReconcilerType is a string which identifies the type of ActionReconciler. diff --git a/internal/reconcile/release_test.go b/internal/reconcile/release_test.go index 038c94309..117820323 100644 --- a/internal/reconcile/release_test.go +++ b/internal/reconcile/release_test.go @@ -17,7 +17,6 @@ limitations under the License. package reconcile import ( - "reflect" "testing" "github.com/go-logr/logr" @@ -36,23 +35,6 @@ const ( mockReleaseNamespace = "mock-ns" ) -func Test_observedReleases_sortedVersions(t *testing.T) { - tests := []struct { - name string - r observedReleases - wantVersions []int - }{ - // TODO: Add test cases. - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if gotVersions := tt.r.sortedVersions(); !reflect.DeepEqual(gotVersions, tt.wantVersions) { - t.Errorf("sortedVersions() = %v, want %v", gotVersions, tt.wantVersions) - } - }) - } -} - func Test_summarize(t *testing.T) { tests := []struct { name string diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index 62154f684..286b8e1b2 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -27,7 +27,7 @@ import ( helmrelease "helm.sh/helm/v3/pkg/release" helmstorage "helm.sh/helm/v3/pkg/storage" helmdriver "helm.sh/helm/v3/pkg/storage/driver" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/fluxcd/pkg/ssa" "github.com/fluxcd/pkg/ssa/jsondiff" @@ -517,12 +517,17 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) { Diff: jsondiff.DiffSet{ { Type: jsondiff.DiffTypeCreate, - GroupVersionKind: schema.GroupVersionKind{ - Kind: "Secret", - Version: "v1", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "fixture", + "namespace": namespace, + "creationTimestamp": nil, + }, + }, }, - Namespace: namespace, - Name: "fixture", }, }, } @@ -545,12 +550,17 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) { Diff: jsondiff.DiffSet{ { Type: jsondiff.DiffTypeCreate, - GroupVersionKind: schema.GroupVersionKind{ - Kind: "Secret", - Version: "v1", + DesiredObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "fixture", + "namespace": namespace, + "creationTimestamp": nil, + }, + }, }, - Namespace: namespace, - Name: "fixture", }, }, } From 0131f2227b1e1f300046020378bd69b8ef412e3f Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 29 Nov 2023 23:01:54 +0100 Subject: [PATCH 2/2] action/diff: include Helm metadata in objects This ensures that the metadata labels and annotations Helm adds during the creation of resources are included while diffing them. As they are not part of the manifest but should be restored in case they are e.g. removed or modified. Signed-off-by: Hidde Beydals --- internal/action/diff.go | 34 +++++++++++++ internal/action/diff_test.go | 86 +++++++++++++++++++------------- internal/reconcile/state_test.go | 21 ++++++++ 3 files changed, 107 insertions(+), 34 deletions(-) diff --git a/internal/action/diff.go b/internal/action/diff.go index e0b835a25..fb330eca9 100644 --- a/internal/action/diff.go +++ b/internal/action/diff.go @@ -68,6 +68,11 @@ func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmreleas errs []error ) for _, obj := range objects { + // Set the Helm metadata on the object which is normally set by Helm + // during object creation. + setHelmMetadata(obj, rls) + + // Set the namespace of the object if it is not set. if obj.GetNamespace() == "" { // Manifest does not contain the namespace of the release. // Figure out if the object is namespaced if the namespace is not @@ -188,6 +193,35 @@ func ApplyDiff(ctx context.Context, config *helmaction.Configuration, diffSet js return changeSet, apierrutil.NewAggregate(errs) } +const ( + appManagedByLabel = "app.kubernetes.io/managed-by" + appManagedByHelm = "Helm" + helmReleaseNameAnnotation = "meta.helm.sh/release-name" + helmReleaseNamespaceAnnotation = "meta.helm.sh/release-namespace" +) + +// setHelmMetadata sets the metadata on the given object to indicate that it is +// managed by Helm. This is safe to do, because we apply it to objects that +// originate from the Helm release itself. +// xref: https://github.com/helm/helm/blob/v3.13.2/pkg/action/validate.go +// xref: https://github.com/helm/helm/blob/v3.13.2/pkg/action/rollback.go#L186-L191 +func setHelmMetadata(obj client.Object, rls *helmrelease.Release) { + labels := obj.GetLabels() + if labels == nil { + labels = make(map[string]string, 1) + } + labels[appManagedByLabel] = appManagedByHelm + obj.SetLabels(labels) + + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string, 2) + } + annotations[helmReleaseNameAnnotation] = rls.Name + annotations[helmReleaseNamespaceAnnotation] = rls.Namespace + obj.SetAnnotations(annotations) +} + // objectToChangeSetEntry returns a ssa.ChangeSetEntry for the given object and // action. func objectToChangeSetEntry(obj client.Object, action ssa.Action) ssa.ChangeSetEntry { diff --git a/internal/action/diff_test.go b/internal/action/diff_test.go index 8face3197..493ce7fb5 100644 --- a/internal/action/diff_test.go +++ b/internal/action/diff_test.go @@ -162,38 +162,6 @@ metadata: name: disabled annotations: %[1]s: %[2]s -data: - key: value`, v2.DriftDetectionMetadataKey, v2.DriftDetectionDisabledValue), - mutateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) { - var clusterObjs []*unstructured.Unstructured - for _, obj := range objs { - obj := obj.DeepCopy() - obj.SetNamespace(namespace) - if err := unstructured.SetNestedField(obj.Object, "changed", "data", "key"); err != nil { - return nil, fmt.Errorf("failed to set nested field: %w", err) - } - clusterObjs = append(clusterObjs, obj) - } - return clusterObjs, nil - }, - want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet { - return jsondiff.DiffSet{ - { - Type: jsondiff.DiffTypeExclude, - DesiredObject: namespacedUnstructured(desired[0], namespace), - }, - } - }, - }, - { - name: "manifest with disabled annotation", - manifest: fmt.Sprintf(`--- -apiVersion: v1 -kind: ConfigMap -metadata: - name: disabled - labels: - %[1]s: %[2]s data: key: value`, v2.DriftDetectionMetadataKey, v2.DriftDetectionDisabledValue), mutateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) { @@ -373,6 +341,53 @@ data: } }, }, + { + name: "configures Helm metadata", + manifest: `--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: without-helm-metadata +data: + key: value`, + mutateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) { + var clusterObjs []*unstructured.Unstructured + for _, obj := range objs { + obj := obj.DeepCopy() + if obj.GetNamespace() == "" { + obj.SetNamespace(namespace) + } + obj.SetAnnotations(nil) + obj.SetLabels(nil) + clusterObjs = append(clusterObjs, obj) + } + return clusterObjs, 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.OperationAdd, + Path: "/metadata", + Value: map[string]interface{}{ + "labels": map[string]interface{}{ + appManagedByLabel: appManagedByHelm, + }, + "annotations": map[string]interface{}{ + helmReleaseNameAnnotation: "configures Helm metadata", + helmReleaseNamespaceAnnotation: namespace, + }, + }, + }, + }, + }, + } + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -389,10 +404,15 @@ data: } }) + rls := &helmrelease.Release{Name: tt.name, Namespace: ns.Name, Manifest: tt.manifest} + objs, err := ssa.ReadObjects(strings.NewReader(tt.manifest)) if err != nil { t.Fatalf("Failed to read release objects: %v", err) } + for _, obj := range objs { + setHelmMetadata(obj, rls) + } clusterObjs := objs if tt.mutateCluster != nil { @@ -418,8 +438,6 @@ data: } } - rls := &helmrelease.Release{Namespace: ns.Name, Manifest: tt.manifest} - got, err := Diff(ctx, &helmaction.Configuration{RESTClientGetter: getter}, rls, testOwner, tt.ignoreRules...) if (err != nil) != tt.wantErr { t.Errorf("Diff() error = %v, wantErr %v", err, tt.wantErr) diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index 286b8e1b2..abc25ef3e 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -525,6 +525,13 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) { "name": "fixture", "namespace": namespace, "creationTimestamp": nil, + "labels": map[string]interface{}{ + "app.kubernetes.io/managed-by": "Helm", + }, + "annotations": map[string]interface{}{ + "meta.helm.sh/release-name": mockReleaseName, + "meta.helm.sh/release-namespace": namespace, + }, }, }, }, @@ -558,6 +565,13 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) { "name": "fixture", "namespace": namespace, "creationTimestamp": nil, + "labels": map[string]interface{}{ + "app.kubernetes.io/managed-by": "Helm", + }, + "annotations": map[string]interface{}{ + "meta.helm.sh/release-name": mockReleaseName, + "meta.helm.sh/release-namespace": namespace, + }, }, }, }, @@ -611,6 +625,13 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) { for _, obj := range objs { g.Expect(ssa.NormalizeUnstructured(obj)).To(Succeed()) obj.SetNamespace(releaseNamespace) + obj.SetLabels(map[string]string{ + "app.kubernetes.io/managed-by": "Helm", + }) + obj.SetAnnotations(map[string]string{ + "meta.helm.sh/release-name": rls.Name, + "meta.helm.sh/release-namespace": rls.Namespace, + }) g.Expect(testEnv.Create(context.Background(), obj)).To(Succeed()) } }