Skip to content

Commit

Permalink
feat(core): track and apply resource changes through deep diffing
Browse files Browse the repository at this point in the history
Up until now, we weren't really doing proper diffing when deciding whether to update
managed resources - we just marked everything as "SYNCED". This patch adds a new delta
package that does a proper deep comparison between desired and observed resource
states, being careful about all the k8s metadata fields that we should ignore during
comparisons (e.g `metadata.generation`, `metadta.revision` etc...)

A key design aspect of this implementation is that `kro` only diffs fields that are
 explicitly defined in `ResourceGroup` - any field that is defaulted by other
controllers or mutating webhooks are ignored. This ensures `kro` coexists seamlessly
with other reconciliation systems without fighting over field ownership.

The delta `Compare` function takes the desired and observed `unstructured.Unstructured`
objects and returns a list of structured "differences", where each difference contains
the full path to the changed field and its desired/observed values. It recursively
walks both object trees in parallel, building pathstring like `spec.containers[0].image`
to precisely identify where values diverge. The comparison handles type mismatches,
nil vs empty maps/slices, value differences etc...

Also patch also adds integration tests in suites/core for generic resource
updates and in suites/ackekscluster for EKS-clutser-specific version updates. note that
in both tests we mainly rely on the `metadata.revision` to validate that indeed the
controller did make an spec update call.
  • Loading branch information
a-hilaly committed Dec 23, 2024
1 parent bfcd4bf commit 5d1e755
Show file tree
Hide file tree
Showing 7 changed files with 914 additions and 13 deletions.
54 changes: 43 additions & 11 deletions internal/controller/instance/controller_reconcile.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/dynamic"

"github.com/awslabs/kro/internal/controller/instance/delta"
"github.com/awslabs/kro/internal/metadata"
"github.com/awslabs/kro/internal/runtime"
"github.com/awslabs/kro/pkg/requeue"
Expand Down Expand Up @@ -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
Expand Down
181 changes: 181 additions & 0 deletions internal/controller/instance/delta/delta.go
Original file line number Diff line number Diff line change
@@ -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
}

// ignoredFields are Kubernetes metadata fields that should not trigger updates.
var ignoredFields = []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 annotations, exists := metadata["labels"].(map[string]interface{}); exists {
if len(annotations) == 0 {
delete(metadata, "labels")
}
}

for _, field := range ignoredFields {
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)
}
}
Loading

0 comments on commit 5d1e755

Please sign in to comment.