Improve topology diff

Author: fabriziopandini

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)

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 apply diff to original object")
+			}
+
+			originalYAMLWithChanges, err := yaml.JSONToYAML(originalJSONWithChanges)
+			if err != nil {
+				return false, false, "", "", errors.Wrapf(err, "failed to apply diff to original object")
+			}
+
+			diffString = cmp.Diff(string(originalYAML), string(originalYAMLWithChanges))
+			diffString = strings.ReplaceAll(diffString, "\u00A0", " ")
+			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

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)

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.