From 7546c62755bfb0ac63aa73cffa054ddf97b29304 Mon Sep 17 00:00:00 2001 From: Jordan Olshevski Date: Wed, 6 Nov 2024 11:51:37 -0600 Subject: [PATCH] Fix patch edgecases (#242) Values that change on the server side should not cause the patch to be continuously applied. There are two known cases where this can occur: bools explicitly set to false, and resource quantities. --------- Co-authored-by: Jordan Olshevski --- .../controllers/reconciliation/controller.go | 27 ++++---- .../reconciliation/controller_test.go | 10 +-- .../controllers/reconciliation/crud_test.go | 64 +++++++++++++++++++ internal/resource/resource.go | 24 +++++++ 4 files changed, 110 insertions(+), 15 deletions(-) diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index ebf39f7e..00c3b5c4 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -305,31 +305,36 @@ func (c *Controller) reconcileResource(ctx context.Context, comp *apiv1.Composit return true, nil } -func (c *Controller) buildPatch(ctx context.Context, prev, resource *reconstitution.Resource, current *unstructured.Unstructured) ([]byte, types.PatchType, error) { - if resource.Patch != nil { - if !resource.NeedsToBePatched(current) { +func (c *Controller) buildPatch(ctx context.Context, prev, next *reconstitution.Resource, current *unstructured.Unstructured) ([]byte, types.PatchType, error) { + if next.Patch != nil { + if !next.NeedsToBePatched(current) { return []byte{}, types.JSONPatchType, nil } - patch, err := json.Marshal(&resource.Patch) + patch, err := json.Marshal(&next.Patch) return patch, types.JSONPatchType, err } - var prevManifest []byte - if prev != nil { - prevManifest = []byte(prev.Manifest.Manifest) + prevJS, err := prev.Finalize() + if err != nil { + return nil, "", reconcile.TerminalError(fmt.Errorf("building json representation of previous state: %w", err)) + } + + nextJS, err := next.Finalize() + if err != nil { + return nil, "", reconcile.TerminalError(fmt.Errorf("building json representation of next state: %w", err)) } currentJS, err := current.MarshalJSON() if err != nil { - return nil, "", reconcile.TerminalError(fmt.Errorf("building json representation of desired state: %w", err)) + return nil, "", reconcile.TerminalError(fmt.Errorf("building json representation of current state: %w", err)) } - model, err := c.discovery.Get(ctx, resource.GVK) + model, err := c.discovery.Get(ctx, next.GVK) if err != nil { return nil, "", fmt.Errorf("getting merge metadata: %w", err) } if model == nil { - patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(prevManifest, []byte(resource.Manifest.Manifest), currentJS) + patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(prevJS, nextJS, currentJS) if err != nil { return nil, "", reconcile.TerminalError(err) } @@ -337,7 +342,7 @@ func (c *Controller) buildPatch(ctx context.Context, prev, resource *reconstitut } patchmeta := strategicpatch.NewPatchMetaFromOpenAPI(model) - patch, err := strategicpatch.CreateThreeWayMergePatch(prevManifest, []byte(resource.Manifest.Manifest), currentJS, patchmeta, true) + patch, err := strategicpatch.CreateThreeWayMergePatch(prevJS, nextJS, currentJS, patchmeta, true) if err != nil { return nil, "", reconcile.TerminalError(err) } diff --git a/internal/controllers/reconciliation/controller_test.go b/internal/controllers/reconciliation/controller_test.go index d5239fb1..4060872d 100644 --- a/internal/controllers/reconciliation/controller_test.go +++ b/internal/controllers/reconciliation/controller_test.go @@ -73,7 +73,7 @@ func TestBuildPatchEmpty(t *testing.T) { "apiVersion": "v1", "kind": "Pod", "metadata": map[string]any{"name": "foo", "namespace": "default"}, - "spec": map[string]any{"serviceAccountName": "initial"}, + "spec": map[string]any{"serviceAccountName": "initial", "containers": nil}, }, }, { @@ -102,12 +102,14 @@ func TestBuildPatchEmpty(t *testing.T) { "kind": "Pod", "metadata": map[string]any{"name": "foo", "namespace": "default"}, "status": map[string]any{"message": "initial"}, + "spec": map[string]any{"containers": nil}, }, Next: map[string]any{ "apiVersion": "v1", "kind": "Pod", "metadata": map[string]any{"name": "foo", "namespace": "default"}, "status": map[string]any{"message": "updated"}, + "spec": map[string]any{"containers": nil}, }, }, { @@ -133,13 +135,13 @@ func TestBuildPatchEmpty(t *testing.T) { "apiVersion": "v1", "kind": "Pod", "metadata": map[string]any{"name": "foo", "namespace": "default"}, - "spec": map[string]any{"serviceAccountName": "initial", "initContainers": []any{}}, + "spec": map[string]any{"serviceAccountName": "initial", "initContainers": []any{}, "containers": nil}, }, Next: map[string]any{ "apiVersion": "v1", "kind": "Pod", "metadata": map[string]any{"name": "foo", "namespace": "default"}, - "spec": map[string]any{"initContainers": []any{}, "serviceAccountName": "initial"}, + "spec": map[string]any{"initContainers": []any{}, "serviceAccountName": "initial", "containers": nil}, }, }, } @@ -158,7 +160,7 @@ func TestBuildPatchEmpty(t *testing.T) { patch, err = mungePatch(patch, "random-rv") require.NoError(t, err) - assert.Nil(t, patch) + assert.Empty(t, string(patch)) assert.Equal(t, test.Type, kind) }) } diff --git a/internal/controllers/reconciliation/crud_test.go b/internal/controllers/reconciliation/crud_test.go index e431ef92..669c7acc 100644 --- a/internal/controllers/reconciliation/crud_test.go +++ b/internal/controllers/reconciliation/crud_test.go @@ -754,3 +754,67 @@ func TestOrphanedCompositionDeletion(t *testing.T) { return errors.IsNotFound(upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)) }) } + +// TestResourceDefaulting proves that resources which define properties equal to the field's default will eventually converge. +func TestResourceDefaulting(t *testing.T) { + scheme := runtime.NewScheme() + corev1.SchemeBuilder.AddToScheme(scheme) + testv1.SchemeBuilder.AddToScheme(scheme) + + ctx := testutil.NewContext(t) + mgr := testutil.NewManager(t) + upstream := mgr.GetClient() + + registerControllers(t, mgr) + testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) { + output := &krmv1.ResourceList{} + output.Items = []*unstructured.Unstructured{{ + Object: map[string]any{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]any{ + "name": "test-obj", + "namespace": "default", + }, + "spec": map[string]any{ + "paused": false, // will fail the test if defaulting isn't handled correctly + "selector": map[string]any{ + "matchLabels": map[string]any{ + "foo": "bar", + }, + }, + "template": map[string]any{ + "metadata": map[string]any{ + "labels": map[string]any{ + "foo": "bar", + }, + }, + "spec": map[string]any{ + "containers": []any{ + map[string]any{ + "name": "foo", + "image": "bar", + "resources": map[string]any{ + "memory": "1024Mi", // apiserver will return this as "1Gi" + }, + }, + }, + }, + }, + }, + }, + }} + return output, nil + }) + + // Test subject + setupTestSubject(t, mgr) + mgr.Start(t) + _, comp := writeGenericComposition(t, upstream) + + // It should be able to become ready + testutil.Eventually(t, func() bool { + err := upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp) + return err == nil && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Ready != nil && comp.Status.CurrentSynthesis.ObservedCompositionGeneration == comp.Generation + }) +} diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 8987cfce..fad5b850 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -1,6 +1,7 @@ package resource import ( + "bytes" "context" "encoding/json" "fmt" @@ -19,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/kubectl/pkg/scheme" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -68,6 +70,28 @@ func (r *Resource) Parse() (*unstructured.Unstructured, error) { return u, u.UnmarshalJSON([]byte(r.Manifest.Manifest)) } +// Finalize converts the resource to its struct representation and returns that value encoded as json. +// If the resource doesn't correspond to a built in type supported by the kubectl scheme the literal manifest is returned. +// +// Note that this means Eno is not completely opaque - it has some "understanding" of the built in types. +// Hopefully we can replace this with the a different approach backed by the openapi spec at some point, +// like github.com/kubernetes-sigs/structured-merge-diff. But I don't think it works for our purposes at the moment. +func (r *Resource) Finalize() ([]byte, error) { + if r == nil { + return nil, nil + } + + typed, _, err := scheme.Codecs.UniversalDeserializer().Decode([]byte(r.Manifest.Manifest), &r.GVK, nil) + if err != nil { + // fall back to unstructured + return []byte(r.Manifest.Manifest), nil + } + + buf := &bytes.Buffer{} + err = unstructured.UnstructuredJSONScheme.Encode(typed, buf) + return buf.Bytes(), err +} + func (r *Resource) FindStatus(slice *apiv1.ResourceSlice) *apiv1.ResourceState { if len(slice.Status.Resources) <= r.ManifestRef.Index { return nil