diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index c6fa0090df73..d7cf5decb7c4 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -524,11 +524,12 @@ func (r *Reconciler) reconcileCluster(ctx context.Context, s *scope.Scope) error return nil } - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info("Patching Cluster") } else { - log.Info("Patching Cluster", "diff", string(changes)) + log.Info("Patching Cluster", "diff", diff, "patch", patchData) } modifiedResourceVersion, err := patchHelper.Patch(ctx) if err != nil { @@ -808,11 +809,12 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, s *scope.Scope return nil } - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info("Patching MachineDeployment") } else { - log.Info("Patching MachineDeployment", "diff", string(changes)) + log.Info("Patching MachineDeployment", "diff", diff, "patch", patchData) } modifiedResourceVersion, err := patchHelper.Patch(ctx) if err != nil { @@ -1072,11 +1074,12 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, mpTo return nil } - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info("Patching MachinePool") } else { - log.Info("Patching MachinePool", "diff", string(changes)) + log.Info("Patching MachinePool", "diff", diff, "patch", patchData) } modifiedResourceVersion, err := patchHelper.Patch(ctx) if err != nil { @@ -1222,11 +1225,12 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile return false, nil } - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind())) } else { - log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind()), "diff", string(changes)) + log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind()), "diff", diff, "patch", patchData) } if _, err := patchHelper.Patch(ctx); err != nil { return false, errors.Wrapf(err, "failed to patch %s %s", in.current.GetKind(), klog.KObj(in.current)) @@ -1317,11 +1321,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci // If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template // rotation we patch the object in place. This avoids recreating machines. if !patchHelper.HasSpecChanges() { - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind())) } else { - log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind()), "diff", string(changes)) + log.Info(fmt.Sprintf("Patching %s", in.desired.GetKind()), "diff", diff, "patch", patchData) } if _, err := patchHelper.Patch(ctx); err != nil { return false, errors.Wrapf(err, "failed to patch %s %s", in.desired.GetKind(), klog.KObj(in.desired)) @@ -1337,11 +1342,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci newName := names.SimpleNameGenerator.GenerateName(in.templateNamePrefix) in.desired.SetName(newName) - changes := patchHelper.Changes() - if len(changes) == 0 { + diff := patchHelper.Diff() + patchData := patchHelper.PatchData() + if diff == "" && patchData == "" { log.Info(fmt.Sprintf("Rotating %s, new name %s", in.current.GetKind(), newName)) } else { - log.Info(fmt.Sprintf("Rotating %s, new name %s", in.current.GetKind(), newName), "diff", string(changes)) + log.Info(fmt.Sprintf("Rotating %s, new name %s", in.current.GetKind(), newName), "diff", diff, "patch", patchData) } log.Info(fmt.Sprintf("Creating %s", in.current.GetKind())) helper, err := structuredmerge.NewServerSidePatchHelper(ctx, nil, in.desired, r.Client, r.ssaCache) diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun.go b/internal/controllers/topology/cluster/structuredmerge/dryrun.go index b2314a2a7a28..f478ee87e6d2 100644 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun.go +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun.go @@ -19,12 +19,15 @@ package structuredmerge import ( "context" "encoding/json" + "strings" jsonpatch "github.com/evanphx/json-patch/v5" + "github.com/google/go-cmp/cmp" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/cluster-api/internal/contract" @@ -47,7 +50,7 @@ type dryRunSSAPatchInput struct { } // dryRunSSAPatch uses server side apply dry run to determine if the operation is going to change the actual object. -func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, bool, []byte, error) { +func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, bool, string, string, error) { // Compute a request identifier. // The identifier is unique for a specific request to ensure we don't have to re-run the request // once we found out that it would not produce a diff. @@ -56,13 +59,13 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // This ensures that we re-run the request as soon as either original or modified changes. requestIdentifier, err := ssa.ComputeRequestIdentifier(dryRunCtx.client.Scheme(), dryRunCtx.originalUnstructured.GetResourceVersion(), dryRunCtx.modifiedUnstructured) if err != nil { - return false, false, nil, err + return false, false, "", "", err } // Check if we already ran this request before by checking if the cache already contains this identifier. // Note: We only add an identifier to the cache if the result of the dry run was no diff. if exists := dryRunCtx.ssaCache.Has(requestIdentifier, dryRunCtx.originalUnstructured.GetKind()); exists { - return false, false, nil, nil + return false, false, "", "", nil } // For dry run we use the same options as for the intent but with adding metadata.managedFields @@ -74,17 +77,17 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // Add TopologyDryRunAnnotation to notify validation webhooks to skip immutability checks. if err := unstructured.SetNestedField(dryRunCtx.originalUnstructured.Object, "", "metadata", "annotations", clusterv1.TopologyDryRunAnnotation); err != nil { - return false, false, nil, errors.Wrap(err, "failed to add topology dry-run annotation to original object") + return false, false, "", "", errors.Wrap(err, "failed to add topology dry-run annotation to original object") } if err := unstructured.SetNestedField(dryRunCtx.modifiedUnstructured.Object, "", "metadata", "annotations", clusterv1.TopologyDryRunAnnotation); err != nil { - return false, false, nil, errors.Wrap(err, "failed to add topology dry-run annotation to modified object") + return false, false, "", "", errors.Wrap(err, "failed to add topology dry-run annotation to modified object") } // Do a server-side apply dry-run with modifiedUnstructured to get the updated object. err = dryRunCtx.client.Apply(ctx, client.ApplyConfigurationFromUnstructured(dryRunCtx.modifiedUnstructured), client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership) if err != nil { // This catches errors like metadata.uid changes. - return false, false, nil, errors.Wrap(err, "server side apply dry-run failed for modified object") + return false, false, "", "", errors.Wrap(err, "server side apply dry-run failed for modified object") } // Do a server-side apply dry-run with originalUnstructured to ensure the latest defaulting is applied. @@ -109,7 +112,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, dryRunCtx.originalUnstructured.SetManagedFields(nil) err = dryRunCtx.client.Apply(ctx, client.ApplyConfigurationFromUnstructured(dryRunCtx.originalUnstructured), client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership) if err != nil { - return false, false, nil, errors.Wrap(err, "server side apply dry-run failed for original object") + return false, false, "", "", errors.Wrap(err, "server side apply dry-run failed for original object") } // Restore managed fields. dryRunCtx.originalUnstructured.SetManagedFields(originalUnstructuredManagedFieldsBeforeSSA) @@ -124,7 +127,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // Please note that if other managers made changes to fields that we care about and thus ownership changed, // this would affect our managed fields as well and we would still detect it by diffing our managed fields. if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.modifiedUnstructured); err != nil { - return false, false, nil, errors.Wrap(err, "failed to filter topology dry-run annotation on modified object") + return false, false, "", "", errors.Wrap(err, "failed to filter topology dry-run annotation on modified object") } // Also run the function for the originalUnstructured to remove the managedField @@ -135,7 +138,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // Please note that if other managers made changes to fields that we care about and thus ownership changed, // this would affect our managed fields as well and we would still detect it by diffing our managed fields. if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.originalUnstructured); err != nil { - return false, false, nil, errors.Wrap(err, "failed to filter topology dry-run annotation on original object") + return false, false, "", "", errors.Wrap(err, "failed to filter topology dry-run annotation on original object") } // Drop the other fields which are not part of our intent. @@ -145,28 +148,28 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // Compare the output of dry run to the original object. originalJSON, err := json.Marshal(dryRunCtx.originalUnstructured) if err != nil { - return false, false, nil, err + return false, false, "", "", err } modifiedJSON, err := json.Marshal(dryRunCtx.modifiedUnstructured) if err != nil { - return false, false, nil, err + return false, false, "", "", err } rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) if err != nil { - return false, false, nil, err + return false, false, "", "", err } // Determine if there are changes to the spec and object. diff := &unstructured.Unstructured{} if err := json.Unmarshal(rawDiff, &diff.Object); err != nil { - return false, false, nil, err + return false, false, "", "", err } hasChanges := len(diff.Object) > 0 _, hasSpecChanges := diff.Object["spec"] - var changes []byte + var patchString, diffString string if hasChanges { // Cleanup diff by dropping .metadata.managedFields. ssa.FilterIntent(&ssa.FilterIntentInput{ @@ -177,10 +180,30 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, // changes should be empty (not "{}") if diff.Object is empty if len(diff.Object) != 0 { - changes, err = json.Marshal(diff.Object) + patchBytes, err := json.Marshal(diff.Object) if err != nil { - return false, false, nil, errors.Wrapf(err, "failed to marshal diff") + return false, false, "", "", errors.Wrapf(err, "failed to marshal diff") } + patchString = string(patchBytes) + + originalJSONWithChanges, err := jsonpatch.MergePatch(originalJSON, patchBytes) + if err != nil { + return false, false, "", "", errors.Wrapf(err, "failed to apply diff to original object") + } + + originalYAML, err := yaml.JSONToYAML(originalJSON) + if err != nil { + return false, false, "", "", errors.Wrapf(err, "failed to convert original object to yaml") + } + + originalYAMLWithChanges, err := yaml.JSONToYAML(originalJSONWithChanges) + if err != nil { + return false, false, "", "", errors.Wrapf(err, "failed to convert original object with diff to yaml") + } + + diffString = cmp.Diff(string(originalYAML), string(originalYAMLWithChanges)) + diffString = strings.ReplaceAll(diffString, "\u00A0", " ") // No-Break Space (NBSP) + diffString = strings.ReplaceAll(diffString, "\t", " ") } } @@ -189,7 +212,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, dryRunCtx.ssaCache.Add(requestIdentifier) } - return hasChanges, hasSpecChanges, changes, nil + return hasChanges, hasSpecChanges, patchString, diffString, nil } // cleanupManagedFieldsAndAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run diff --git a/internal/controllers/topology/cluster/structuredmerge/interfaces.go b/internal/controllers/topology/cluster/structuredmerge/interfaces.go index 61e99c81dc70..15c6ad915ccc 100644 --- a/internal/controllers/topology/cluster/structuredmerge/interfaces.go +++ b/internal/controllers/topology/cluster/structuredmerge/interfaces.go @@ -37,8 +37,11 @@ type PatchHelper interface { // HasSpecChanges return true if the modified object is generating spec changes vs the original object. HasSpecChanges() bool - // Changes returns the changes vs the original object. - Changes() []byte + // PatchData return the patch that will be applied. + PatchData() string + + // Diff return the diff between original and modified. + Diff() string // Patch patches the given obj in the Kubernetes cluster. Patch(ctx context.Context) (modifiedResourceVersion string, err error) diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index 475a258aa8cd..54c927a9d4ec 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -36,7 +36,8 @@ type serverSidePatchHelper struct { modified *unstructured.Unstructured hasChanges bool hasSpecChanges bool - changes []byte + patch string + diff string } // NewServerSidePatchHelper returns a new PatchHelper using server side apply. @@ -90,13 +91,13 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj // Determine if the intent defined in the modified object is going to trigger // an actual change when running server side apply, and if this change might impact the object spec or not. var hasChanges, hasSpecChanges bool - var changes []byte + var patch, diff string switch { case util.IsNil(original): hasChanges, hasSpecChanges = true, true default: var err error - hasChanges, hasSpecChanges, changes, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{ + hasChanges, hasSpecChanges, patch, diff, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{ client: c, ssaCache: ssaCache, originalUnstructured: originalUnstructured, @@ -113,7 +114,8 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj modified: modifiedUnstructured, hasChanges: hasChanges, hasSpecChanges: hasSpecChanges, - changes: changes, + patch: patch, + diff: diff, }, nil } @@ -122,9 +124,14 @@ func (h *serverSidePatchHelper) HasSpecChanges() bool { return h.hasSpecChanges } -// Changes return the changes. -func (h *serverSidePatchHelper) Changes() []byte { - return h.changes +// PatchData return the patch that will be applied. +func (h *serverSidePatchHelper) PatchData() string { + return h.patch +} + +// Diff return the diff between original and modified. +func (h *serverSidePatchHelper) Diff() string { + return h.diff } // HasChanges return true if the patch has changes. diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 85c661abd465..5382ae6d50c5 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "testing" "time" @@ -79,7 +80,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create. + g.Expect(p0.PatchData()).To(BeEmpty()) // changes are expected to be empty on create. + g.Expect(p0.Diff()).To(BeEmpty()) // changes are expected to be empty on create. }) t.Run("Server side apply detect changes on object creation (typed)", func(t *testing.T) { g := NewWithT(t) @@ -91,7 +93,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create. + g.Expect(p0.PatchData()).To(BeEmpty()) // changes are expected to be empty on create. + g.Expect(p0.Diff()).To(BeEmpty()) // changes are expected to be empty on create. }) t.Run("When creating an object using server side apply, it should track managed fields for the topology controller", func(t *testing.T) { g := NewWithT(t) @@ -101,7 +104,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create. + g.Expect(p0.PatchData()).To(BeEmpty()) // changes are expected to be empty on create. + g.Expect(p0.Diff()).To(BeEmpty()) // changes are expected to be empty on create. // Create the object using server side apply _, err = p0.Patch(ctx) @@ -138,7 +142,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) }) t.Run("Server side apply patch helper discard changes in not allowed fields, e.g. status", func(t *testing.T) { @@ -156,7 +161,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) }) t.Run("Server side apply patch helper detect changes", func(t *testing.T) { @@ -174,7 +180,19 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"bar":"changed"}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"spec":{"bar":"changed"}}`)) + g.Expect(p0.Diff()).To(Equal(strings.ReplaceAll(` ( + """ + ... // 15 identical lines + uid: + spec: ++ bar: changed + controlPlaneEndpoint: + host: 1.2.3.4 + ... // 2 identical lines + """ + ) +`, "", string(original.GetUID())))) }) t.Run("Server side apply patch helper detect changes impacting only metadata.labels", func(t *testing.T) { @@ -192,7 +210,20 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"labels":{"foo":"changed"}}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"metadata":{"labels":{"foo":"changed"}}}`)) + g.Expect(p0.Diff()).To(Equal(` ( + """ + apiVersion: infrastructure.cluster.x-k8s.io/v1beta2 + kind: TestInfrastructureCluster + metadata: ++ labels: ++ foo: changed + managedFields: + - apiVersion: infrastructure.cluster.x-k8s.io/v1beta2 + ... // 16 identical lines + """ + ) +`)) }) t.Run("Server side apply patch helper detect changes impacting only metadata.annotations", func(t *testing.T) { @@ -210,7 +241,20 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"annotations":{"foo":"changed"}}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"metadata":{"annotations":{"foo":"changed"}}}`)) + g.Expect(p0.Diff()).To(Equal(` ( + """ + apiVersion: infrastructure.cluster.x-k8s.io/v1beta2 + kind: TestInfrastructureCluster + metadata: ++ annotations: ++ foo: changed + managedFields: + - apiVersion: infrastructure.cluster.x-k8s.io/v1beta2 + ... // 16 identical lines + """ + ) +`)) }) t.Run("Server side apply patch helper detect changes impacting only metadata.ownerReferences", func(t *testing.T) { @@ -235,7 +279,23 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"ownerReferences":[{"apiVersion":"foo/v1alpha1","kind":"foo","name":"foo","uid":"foo"}]}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"metadata":{"ownerReferences":[{"apiVersion":"foo/v1alpha1","kind":"foo","name":"foo","uid":"foo"}]}}`)) + g.Expect(p0.Diff()).To(Equal(strings.ReplaceAll(strings.ReplaceAll(` ( + """ + ... // 13 identical lines + name: obj1 + namespace: ++ ownerReferences: ++ - apiVersion: foo/v1alpha1 ++ kind: foo ++ name: foo ++ uid: foo + uid: + spec: + ... // 4 identical lines + """ + ) +`, "", original.GetNamespace()), "", string(original.GetUID())))) }) t.Run("Server side apply patch helper discard changes in ignore paths", func(t *testing.T) { @@ -253,7 +313,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) }) t.Run("Another controller applies changes", func(t *testing.T) { @@ -287,7 +348,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) }) t.Run("Topology controller reconcile again with no changes on topology managed fields", func(t *testing.T) { @@ -304,7 +366,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) // Change the object using server side apply _, err = p0.Patch(ctx) @@ -353,7 +416,18 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"controlPlaneEndpoint":{"host":"changed"}}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"spec":{"controlPlaneEndpoint":{"host":"changed"}}}`)) + g.Expect(p0.Diff()).To(Equal(` ( + """ + ... // 17 identical lines + bar: changed + controlPlaneEndpoint: +- host: 1.2.3.4 ++ host: changed + port: 1234 + """ + ) +`)) // Create the object using server side apply _, err = p0.Patch(ctx) @@ -393,7 +467,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeEmpty()) // Note: metadata.managedFields have been removed from the diff to reduce log verbosity. + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) // Note: metadata.managedFields have been removed from the diff to reduce log verbosity. // Create the object using server side apply _, err = p0.Patch(ctx) @@ -435,7 +510,20 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"bar":"changed-by-topology-controller"}}`))) + g.Expect(p0.PatchData()).To(Equal(`{"spec":{"bar":"changed-by-topology-controller"}}`)) + g.Expect(p0.Diff()).To(Equal(strings.ReplaceAll(` ( + """ + ... // 16 identical lines + uid: + spec: +- bar: changed ++ bar: changed-by-topology-controller + controlPlaneEndpoint: + host: changed + ... // 2 identical lines + """ + ) +`, "", string(original.GetUID())))) // Create the object using server side apply _, err = p0.Patch(ctx) @@ -485,7 +573,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - g.Expect(p0.Changes()).To(BeNil()) + g.Expect(p0.PatchData()).To(BeEmpty()) + g.Expect(p0.Diff()).To(BeEmpty()) }) t.Run("Error on object which has another uid due to immutability", func(t *testing.T) { g := NewWithT(t) @@ -735,7 +824,7 @@ func TestServerSideApplyWithDefaulting(t *testing.T) { // Apply modified. p0, err = NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), ssaCache) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(p0.HasChanges()).To(Equal(tt.expectChanges), fmt.Sprintf("changes: %s", string(p0.Changes()))) + g.Expect(p0.HasChanges()).To(Equal(tt.expectChanges), fmt.Sprintf("changes: %s", p0.Diff())) g.Expect(p0.HasSpecChanges()).To(Equal(tt.expectSpecChanges)) _, err = p0.Patch(ctx) g.Expect(err).ToNot(HaveOccurred())