Merge pull request #173 from a-hilaly/instance-upadte-path

feat(core): track and apply resource changes through deep diffing

Author: michaelhtm

pkg/controller/instance/controller_reconcile.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 
+	"github.com/awslabs/kro/pkg/controller/instance/delta"
 	"github.com/awslabs/kro/pkg/metadata"
 	"github.com/awslabs/kro/pkg/requeue"
 	"github.com/awslabs/kro/pkg/runtime"
@@ -232,23 +233,54 @@ func (igr *instanceGraphReconciler) handleResourceCreation(
 	return igr.delayedRequeue(fmt.Errorf("awaiting resource creation completion"))
 }
 
-// updateResource handles updates to an existing resource.
-// Currently performs basic state management, but could be extended to include
-// more sophisticated update logic and diffing.
+// updateResource handles updates to an existing resource, comparing the desired
+// and observed states and applying the necessary changes.
 func (igr *instanceGraphReconciler) updateResource(
-	_ context.Context,
-	_ dynamic.ResourceInterface,
-	_, _ *unstructured.Unstructured,
+	ctx context.Context,
+	rc dynamic.ResourceInterface,
+	desired, observed *unstructured.Unstructured,
 	resourceID string,
 	resourceState *ResourceState,
 ) error {
-	igr.log.V(1).Info("Processing potential resource update", "resourceID", resourceID)
+	igr.log.V(1).Info("Processing resource update", "resourceID", resourceID)
 
-	// TODO: Implement resource diffing logic
-	// TODO: Add update strategy options (e.g., server-side apply)
+	// Compare desired and observed states
+	differences, err := delta.Compare(desired, observed)
+	if err != nil {
+		resourceState.State = "ERROR"
+		resourceState.Err = fmt.Errorf("failed to compare desired and observed states: %w", err)
+		return resourceState.Err
+	}
 
-	resourceState.State = "SYNCED"
-	return nil
+	// If no differences are found, the resource is in sync.
+	if len(differences) == 0 {
+		resourceState.State = "SYNCED"
+		igr.log.V(1).Info("No deltas found for resource", "resourceID", resourceID)
+		return nil
+	}
+
+	// Proceed with the update, note that we don't need to handle each difference
+	// individually. We can apply all changes at once.
+	//
+	// NOTE(a-hilaly): are there any cases where we need to handle each difference individually?
+	igr.log.V(1).Info("Found deltas for resource",
+		"resourceID", resourceID,
+		"delta", differences,
+	)
+	igr.instanceSubResourcesLabeler.ApplyLabels(desired)
+
+	// Apply changes to the resource
+	desired.SetResourceVersion(observed.GetResourceVersion())
+	_, err = rc.Update(ctx, desired, metav1.UpdateOptions{})
+	if err != nil {
+		resourceState.State = "ERROR"
+		resourceState.Err = fmt.Errorf("failed to update resource: %w", err)
+		return resourceState.Err
+	}
+
+	// Set state to UPDATING and requeue to check the update
+	resourceState.State = "UPDATING"
+	return igr.delayedRequeue(fmt.Errorf("resource update in progress"))
 }
 
 // handleInstanceDeletion manages the deletion of an instance and its resources

pkg/controller/instance/delta/delta.go

@@ -0,0 +1,181 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file 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 delta
+
+import (
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+// Difference represents a single field-level difference between two objects.
+// Path is the full path to the differing field (e.g. "spec.containers[0].image")
+// Desired and Observed contain the actual values that differ at that path.
+type Difference struct {
+	// Path is the full path to the differing field (e.g. "spec.x.y.z"
+	Path string `json:"path"`
+	// Desired is the desired value at the path
+	Desired interface{} `json:"desired"`
+	// Observed is the observed value at the path
+	Observed interface{} `json:"observed"`
+}
+
+// Compare takes desired and observed unstructured objects and returns a list of
+// their differences. It performs a deep comparison while being aware of Kubernetes
+// metadata specifics. The comparison:
+//
+// - Cleans metadata from both objects to avoid spurious differences
+// - Walks object trees in parallel to find actual value differences
+// - Builds path strings to precisely identify where differences occurs
+// - Handles type mismatches, nil values, and empty vs nil collections
+func Compare(desired, observed *unstructured.Unstructured) ([]Difference, error) {
+	desiredCopy := desired.DeepCopy()
+	observedCopy := observed.DeepCopy()
+
+	cleanMetadata(desiredCopy)
+	cleanMetadata(observedCopy)
+
+	var differences []Difference
+	walkCompare(desiredCopy.Object, observedCopy.Object, "", &differences)
+	return differences, nil
+}
+
+// ignoredMetadataFields are Kubernetes metadata fields that should not trigger updates.
+var ignoredMetadataFields = []string{
+	"creationTimestamp",
+	"deletionTimestamp",
+	"generation",
+	"resourceVersion",
+	"selfLink",
+	"uid",
+	"managedFields",
+	"ownerReferences",
+	"finalizers",
+}
+
+// cleanMetadata removes Kubernetes metadata fields that should not trigger updates
+// like resourceVersion, creationTimestamp, etc. Also handles empty maps in
+// annotations and labels. This ensures we don't detect spurious changes based on
+// Kubernetes-managed fields.
+func cleanMetadata(obj *unstructured.Unstructured) {
+	metadata, ok := obj.Object["metadata"].(map[string]interface{})
+	if !ok {
+		// Maybe we should panic here, but for now just return
+		return
+	}
+
+	if annotations, exists := metadata["annotations"].(map[string]interface{}); exists {
+		if len(annotations) == 0 {
+			delete(metadata, "annotations")
+		}
+	}
+
+	if labels, exists := metadata["labels"].(map[string]interface{}); exists {
+		if len(labels) == 0 {
+			delete(metadata, "labels")
+		}
+	}
+
+	for _, field := range ignoredMetadataFields {
+		delete(metadata, field)
+	}
+}
+
+// walkCompare recursively compares desired and observed values, recording any
+// differences found. It handles different types appropriately:
+// - For maps: recursively compares all keys/values
+// - For slices: checks length and recursively compares elements
+// - For primitives: directly compares values
+//
+// Records a Difference if values don't match or are of different types.
+func walkCompare(desired, observed interface{}, path string, differences *[]Difference) {
+	switch d := desired.(type) {
+	case map[string]interface{}:
+		e, ok := observed.(map[string]interface{})
+		if !ok {
+			*differences = append(*differences, Difference{
+				Path:     path,
+				Observed: observed,
+				Desired:  desired,
+			})
+			return
+		}
+		walkMap(d, e, path, differences)
+
+	case []interface{}:
+		e, ok := observed.([]interface{})
+		if !ok {
+			*differences = append(*differences, Difference{
+				Path:     path,
+				Observed: observed,
+				Desired:  desired,
+			})
+			return
+		}
+		walkSlice(d, e, path, differences)
+
+	default:
+		if desired != observed {
+			*differences = append(*differences, Difference{
+				Path:     path,
+				Observed: observed,
+				Desired:  desired,
+			})
+		}
+	}
+}
+
+// walkMap compares two maps recursively. For each key in desired:
+//
+// - If key missing in observed: records a difference
+// - If key exists: recursively compares values
+func walkMap(desired, observed map[string]interface{}, path string, differences *[]Difference) {
+	for k, desiredVal := range desired {
+		newPath := k
+		if path != "" {
+			newPath = fmt.Sprintf("%s.%s", path, k)
+		}
+
+		observedVal, exists := observed[k]
+		if !exists && desiredVal != nil {
+			*differences = append(*differences, Difference{
+				Path:     newPath,
+				Observed: nil,
+				Desired:  desiredVal,
+			})
+			continue
+		}
+
+		walkCompare(desiredVal, observedVal, newPath, differences)
+	}
+}
+
+// walkSlice compares two slices recursively:
+// - If lengths differ: records entire slice as different
+// - If lengths match: recursively compares elements
+func walkSlice(desired, observed []interface{}, path string, differences *[]Difference) {
+	if len(desired) != len(observed) {
+		*differences = append(*differences, Difference{
+			Path:     path,
+			Observed: observed,
+			Desired:  desired,
+		})
+		return
+	}
+
+	for i := range desired {
+		newPath := fmt.Sprintf("%s[%d]", path, i)
+		walkCompare(desired[i], observed[i], newPath, differences)
+	}
+}