Skip to content

Commit bfe9687

Browse files
committed
feat(core): track and apply resource changes through deep diffing
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.
1 parent 2d1a2f8 commit bfe9687

File tree

7 files changed

+914
-13
lines changed

7 files changed

+914
-13
lines changed

pkg/controller/instance/controller_reconcile.go

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"k8s.io/apimachinery/pkg/types"
2626
"k8s.io/client-go/dynamic"
2727

28+
"github.com/awslabs/kro/pkg/controller/instance/delta"
2829
"github.com/awslabs/kro/pkg/metadata"
2930
"github.com/awslabs/kro/pkg/requeue"
3031
"github.com/awslabs/kro/pkg/runtime"
@@ -232,23 +233,54 @@ func (igr *instanceGraphReconciler) handleResourceCreation(
232233
return igr.delayedRequeue(fmt.Errorf("awaiting resource creation completion"))
233234
}
234235

235-
// updateResource handles updates to an existing resource.
236-
// Currently performs basic state management, but could be extended to include
237-
// more sophisticated update logic and diffing.
236+
// updateResource handles updates to an existing resource, comparing the desired
237+
// and observed states and applying the necessary changes.
238238
func (igr *instanceGraphReconciler) updateResource(
239-
_ context.Context,
240-
_ dynamic.ResourceInterface,
241-
_, _ *unstructured.Unstructured,
239+
ctx context.Context,
240+
rc dynamic.ResourceInterface,
241+
desired, observed *unstructured.Unstructured,
242242
resourceID string,
243243
resourceState *ResourceState,
244244
) error {
245-
igr.log.V(1).Info("Processing potential resource update", "resourceID", resourceID)
245+
igr.log.V(1).Info("Processing resource update", "resourceID", resourceID)
246246

247-
// TODO: Implement resource diffing logic
248-
// TODO: Add update strategy options (e.g., server-side apply)
247+
// Compare desired and observed states
248+
differences, err := delta.Compare(desired, observed)
249+
if err != nil {
250+
resourceState.State = "ERROR"
251+
resourceState.Err = fmt.Errorf("failed to compare desired and observed states: %w", err)
252+
return resourceState.Err
253+
}
249254

250-
resourceState.State = "SYNCED"
251-
return nil
255+
// If no differences are found, the resource is in sync.
256+
if len(differences) == 0 {
257+
resourceState.State = "SYNCED"
258+
igr.log.V(1).Info("No deltas found for resource", "resourceID", resourceID)
259+
return nil
260+
}
261+
262+
// Proceed with the update, note that we don't need to handle each difference
263+
// individually. We can apply all changes at once.
264+
//
265+
// NOTE(a-hilaly): are there any cases where we need to handle each difference individually?
266+
igr.log.V(1).Info("Found deltas for resource",
267+
"resourceID", resourceID,
268+
"delta", differences,
269+
)
270+
igr.instanceSubResourcesLabeler.ApplyLabels(desired)
271+
272+
// Apply changes to the resource
273+
desired.SetResourceVersion(observed.GetResourceVersion())
274+
_, err = rc.Update(ctx, desired, metav1.UpdateOptions{})
275+
if err != nil {
276+
resourceState.State = "ERROR"
277+
resourceState.Err = fmt.Errorf("failed to update resource: %w", err)
278+
return resourceState.Err
279+
}
280+
281+
// Set state to UPDATING and requeue to check the update
282+
resourceState.State = "UPDATING"
283+
return igr.delayedRequeue(fmt.Errorf("resource update in progress"))
252284
}
253285

254286
// handleInstanceDeletion manages the deletion of an instance and its resources
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"). You may
4+
// not use this file except in compliance with the License. A copy of the
5+
// License is located at
6+
//
7+
// http://aws.amazon.com/apache2.0/
8+
//
9+
// or in the "license" file accompanying this file. This file is distributed
10+
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
11+
// express or implied. See the License for the specific language governing
12+
// permissions and limitations under the License.
13+
14+
package delta
15+
16+
import (
17+
"fmt"
18+
19+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
20+
)
21+
22+
// Difference represents a single field-level difference between two objects.
23+
// Path is the full path to the differing field (e.g. "spec.containers[0].image")
24+
// Desired and Observed contain the actual values that differ at that path.
25+
type Difference struct {
26+
// Path is the full path to the differing field (e.g. "spec.x.y.z"
27+
Path string `json:"path"`
28+
// Desired is the desired value at the path
29+
Desired interface{} `json:"desired"`
30+
// Observed is the observed value at the path
31+
Observed interface{} `json:"observed"`
32+
}
33+
34+
// Compare takes desired and observed unstructured objects and returns a list of
35+
// their differences. It performs a deep comparison while being aware of Kubernetes
36+
// metadata specifics. The comparison:
37+
//
38+
// - Cleans metadata from both objects to avoid spurious differences
39+
// - Walks object trees in parallel to find actual value differences
40+
// - Builds path strings to precisely identify where differences occurs
41+
// - Handles type mismatches, nil values, and empty vs nil collections
42+
func Compare(desired, observed *unstructured.Unstructured) ([]Difference, error) {
43+
desiredCopy := desired.DeepCopy()
44+
observedCopy := observed.DeepCopy()
45+
46+
cleanMetadata(desiredCopy)
47+
cleanMetadata(observedCopy)
48+
49+
var differences []Difference
50+
walkCompare(desiredCopy.Object, observedCopy.Object, "", &differences)
51+
return differences, nil
52+
}
53+
54+
// ignoredMetadataFields are Kubernetes metadata fields that should not trigger updates.
55+
var ignoredMetadataFields = []string{
56+
"creationTimestamp",
57+
"deletionTimestamp",
58+
"generation",
59+
"resourceVersion",
60+
"selfLink",
61+
"uid",
62+
"managedFields",
63+
"ownerReferences",
64+
"finalizers",
65+
}
66+
67+
// cleanMetadata removes Kubernetes metadata fields that should not trigger updates
68+
// like resourceVersion, creationTimestamp, etc. Also handles empty maps in
69+
// annotations and labels. This ensures we don't detect spurious changes based on
70+
// Kubernetes-managed fields.
71+
func cleanMetadata(obj *unstructured.Unstructured) {
72+
metadata, ok := obj.Object["metadata"].(map[string]interface{})
73+
if !ok {
74+
// Maybe we should panic here, but for now just return
75+
return
76+
}
77+
78+
if annotations, exists := metadata["annotations"].(map[string]interface{}); exists {
79+
if len(annotations) == 0 {
80+
delete(metadata, "annotations")
81+
}
82+
}
83+
84+
if labels, exists := metadata["labels"].(map[string]interface{}); exists {
85+
if len(labels) == 0 {
86+
delete(metadata, "labels")
87+
}
88+
}
89+
90+
for _, field := range ignoredMetadataFields {
91+
delete(metadata, field)
92+
}
93+
}
94+
95+
// walkCompare recursively compares desired and observed values, recording any
96+
// differences found. It handles different types appropriately:
97+
// - For maps: recursively compares all keys/values
98+
// - For slices: checks length and recursively compares elements
99+
// - For primitives: directly compares values
100+
//
101+
// Records a Difference if values don't match or are of different types.
102+
func walkCompare(desired, observed interface{}, path string, differences *[]Difference) {
103+
switch d := desired.(type) {
104+
case map[string]interface{}:
105+
e, ok := observed.(map[string]interface{})
106+
if !ok {
107+
*differences = append(*differences, Difference{
108+
Path: path,
109+
Observed: observed,
110+
Desired: desired,
111+
})
112+
return
113+
}
114+
walkMap(d, e, path, differences)
115+
116+
case []interface{}:
117+
e, ok := observed.([]interface{})
118+
if !ok {
119+
*differences = append(*differences, Difference{
120+
Path: path,
121+
Observed: observed,
122+
Desired: desired,
123+
})
124+
return
125+
}
126+
walkSlice(d, e, path, differences)
127+
128+
default:
129+
if desired != observed {
130+
*differences = append(*differences, Difference{
131+
Path: path,
132+
Observed: observed,
133+
Desired: desired,
134+
})
135+
}
136+
}
137+
}
138+
139+
// walkMap compares two maps recursively. For each key in desired:
140+
//
141+
// - If key missing in observed: records a difference
142+
// - If key exists: recursively compares values
143+
func walkMap(desired, observed map[string]interface{}, path string, differences *[]Difference) {
144+
for k, desiredVal := range desired {
145+
newPath := k
146+
if path != "" {
147+
newPath = fmt.Sprintf("%s.%s", path, k)
148+
}
149+
150+
observedVal, exists := observed[k]
151+
if !exists && desiredVal != nil {
152+
*differences = append(*differences, Difference{
153+
Path: newPath,
154+
Observed: nil,
155+
Desired: desiredVal,
156+
})
157+
continue
158+
}
159+
160+
walkCompare(desiredVal, observedVal, newPath, differences)
161+
}
162+
}
163+
164+
// walkSlice compares two slices recursively:
165+
// - If lengths differ: records entire slice as different
166+
// - If lengths match: recursively compares elements
167+
func walkSlice(desired, observed []interface{}, path string, differences *[]Difference) {
168+
if len(desired) != len(observed) {
169+
*differences = append(*differences, Difference{
170+
Path: path,
171+
Observed: observed,
172+
Desired: desired,
173+
})
174+
return
175+
}
176+
177+
for i := range desired {
178+
newPath := fmt.Sprintf("%s[%d]", path, i)
179+
walkCompare(desired[i], observed[i], newPath, differences)
180+
}
181+
}

0 commit comments

Comments
 (0)