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 <[email protected]>
Azure · Nov 6, 2024 · 7546c62 · 7546c62
1 parent 6f3ec6b
commit 7546c62
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 15 deletions.
diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go
@@ -305,39 +305,44 @@ 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)
 		}
 		return patch, types.MergePatchType, err
 	}
 
 	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
@@ -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
@@ -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
@@ -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