Repository: crossplane/crossplane-runtime

Issue:
Reciliation storm when using listType=set
<!--
Thank you for helping to improve Crossplane!

Please be sure to search for open issues before raising a new one. We use issues
for bug reports and feature requests. Please find us at https://slack.crossplane.io
for questions, support, and discussion.
-->

### What happened?
<!--
Please let us know what behaviour you expected and how Crossplane diverged from
that behaviour.
-->

When using `Firewall` from Linode provider. We get a reconciliation loop on the resource.

<details>

```
start watching the diff of firewalls on f-agent @Now=2025-01-14 16:05:02.770090568 +0100 CET m=+1.977327563
# Resource: kubectl get firewalls -oyaml f-agent @ 2025-01-14 16:05:14.996056599 +0100 CET m=+14.203293609
--- old/firewalls/f-agent.yaml
+++ new/firewalls/f-agent.yaml
@@ -12,7 +12,7 @@
     creationTimestamp: "2025-01-07T10:16:42Z"
     finalizers:
     - finalizer.managedresource.crossplane.io
-    generation: 479260
+    generation: 479261
     labels:
@@ -30,7 +30,7 @@
             f:linodesRefs: {}
       manager: managed.crossplane.io/api-simple-reference-resolver
       operation: Apply
-      time: "2025-01-14T15:04:52Z"
+      time: "2025-01-14T15:05:14Z"
     - apiVersion: firewall.linode.upbound.io/v1alpha1
       fieldsType: FieldsV1
       fieldsV1:
@@ -146,7 +146,7 @@
       operation: Update
       time: "2025-01-14T11:26:31Z"
     name: f-agent
-    resourceVersion: "8968519"
+    resourceVersion: "8968817"
     uid: 3cd8a430-1eef-40a5-af88-f523418ed1ec
   spec:
     deletionPolicy: Delete
@@ -640,13 +640,13 @@
       inboundPolicy: DROP
       label: f-agent
       linodes:
+      - 2
       - 4
       - 6
-      - 2
       linodesRefs:
+      - name: f-default-2
       - name: f-default-0
       - name: f-default-1
-      - name: f-default-2
       linodesSelector:
         matchLabels:
           label: label
# Resource: kubectl get firewalls -oyaml f-agent @ 2025-01-14 16:05:15.931564636 +0100 CET m=+15.138801639
--- old/firewalls/f-agent.yaml
+++ new/firewalls/f-agent.yaml
@@ -12,7 +12,7 @@
     creationTimestamp: "2025-01-07T10:16:42Z"
     finalizers:
     - finalizer.managedresource.crossplane.io
-    generation: 479261
+    generation: 479262
     labels:
@@ -30,7 +30,7 @@
             f:linodesRefs: {}
       manager: managed.crossplane.io/api-simple-reference-resolver
       operation: Apply
-      time: "2025-01-14T15:05:14Z"
+      time: "2025-01-14T15:05:15Z"
     - apiVersion: firewall.linode.upbound.io/v1alpha1
       fieldsType: FieldsV1
       fieldsV1:
@@ -146,7 +146,7 @@
       operation: Update
       time: "2025-01-14T11:26:31Z"
     name: f-agent
-    resourceVersion: "8968817"
+    resourceVersion: "8968838"
     uid: 3cd8a430-1eef-40a5-af88-f523418ed1ec
   spec:
     deletionPolicy: Delete
@@ -640,13 +640,13 @@
       inboundPolicy: DROP
       label: f-agent
       linodes:
-      - 2
       - 4
       - 6
+      - 2
       linodesRefs:
-      - name: f-default-2
       - name: f-default-0
       - name: f-default-1
+      - name: f-default-2
       linodesSelector:
         matchLabels:
           label: label
# Resource: kubectl get firewalls -oyaml f-agent @ 2025-01-14 16:05:18.839003021 +0100 CET m=+18.046240040
--- old/firewalls/f-agent.yaml
+++ new/firewalls/f-agent.yaml
@@ -12,7 +12,7 @@
     creationTimestamp: "2025-01-07T10:16:42Z"
     finalizers:
     - finalizer.managedresource.crossplane.io
-    generation: 479262
+    generation: 479263
     labels:
@@ -30,7 +30,7 @@
             f:linodesRefs: {}
       manager: managed.crossplane.io/api-simple-reference-resolver
       operation: Apply
-      time: "2025-01-14T15:05:15Z"
+      time: "2025-01-14T15:05:18Z"
     - apiVersion: firewall.linode.upbound.io/v1alpha1
       fieldsType: FieldsV1
       fieldsV1:
@@ -146,7 +146,7 @@
       operation: Update
       time: "2025-01-14T11:26:31Z"
     name: f-agent
-    resourceVersion: "8968838"
+    resourceVersion: "8968861"
     uid: 3cd8a430-1eef-40a5-af88-f523418ed1ec
   spec:
     deletionPolicy: Delete
@@ -640,13 +640,13 @@
       inboundPolicy: DROP
       linodes:
+      - 2
       - 4
       - 6
-      - 2
       linodesRefs:
+      - name: f-default-2
       - name: f-default-0
       - name: f-default-1
-      - name: f-default-2
       linodesSelector:
         matchLabels:
           cluster: demo-cluster
# Resource: kubectl get firewalls -oyaml f-agent @ 2025-01-14 16:05:19.328442838 +0100 CET m=+18.535679850
--- old/firewalls/f-agent.yaml
+++ new/firewalls/f-agent.yaml
@@ -12,7 +12,7 @@
     creationTimestamp: "2025-01-07T10:16:42Z"
     finalizers:
     - finalizer.managedresource.crossplane.io
-    generation: 479263
+    generation: 479264
     labels:
@@ -30,7 +30,7 @@
             f:linodesRefs: {}
       manager: managed.crossplane.io/api-simple-reference-resolver
       operation: Apply
-      time: "2025-01-14T15:05:18Z"
+      time: "2025-01-14T15:05:19Z"
     - apiVersion: firewall.linode.upbound.io/v1alpha1
       fieldsType: FieldsV1
       fieldsV1:
@@ -146,7 +146,7 @@
       operation: Update
       time: "2025-01-14T11:26:31Z"
     name: f-agent
-    resourceVersion: "8968861"
+    resourceVersion: "8968869"
     uid: 3cd8a430-1eef-40a5-af88-f523418ed1ec
   spec:
     deletionPolicy: Delete
@@ -640,13 +640,13 @@
       inboundPolicy: DROP
       linodes:
-      - 2
       - 4
       - 6
+      - 2
       linodesRefs:
-      - name: f-default-2
       - name: f-default-0
       - name: f-default-1
+      - name: f-default-2
       linodesSelector:
         matchLabels:
           label: label
```

</details>

### How can we reproduce it?
<!--
Help us to reproduce your bug as succinctly and precisely as possible. Artifacts
such as example manifests or a script that triggers the issue are highly
appreciated!
-->

Create a firewall with a `linodesSelector`.
Use kubectl-watch to see loop on `linodes` and `linodesRefs`.

### What environment did it happen in?
Crossplane version: 1.16.0

<!--
Include at least the version or commit of Crossplane you were running. Consider
also including your:

* Cloud provider or hardware configuration
* Kubernetes version (use `kubectl version`)
* Kubernetes distribution (e.g. Tectonic, GKE, OpenShift)
* OS (e.g. from /etc/os-release)
* Kernel (e.g. `uname -a`)
-->


Discussion:

Solution patch:
diff --git a/go.sum b/go.sum
index 49d4fe7..2143fae 100644
--- a/go.sum
+++ b/go.sum
@@ -6,7 +6,6 @@ github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
 github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
-github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
 github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -115,12 +114,8 @@ github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/spf13/afero v1.11.0 h1:WJQKhtpdm3v2IzqG8VMqrr6Rf3UYpEF239Jy9wNepM8=
 github.com/spf13/afero v1.11.0/go.mod h1:GH9Y3pIexgf1MTIWtNGyogA5MwRIDXGUr+hbWNoBjkY=
-github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM=
-github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y=
 github.com/spf13/cobra v1.9.1 h1:CXSaggrXdbHK9CF+8ywj8Amf7PBRmPCOJugH954Nnlo=
 github.com/spf13/cobra v1.9.1/go.mod h1:nDyEzZ8ogv936Cinf6g1RU9MRY64Ir93oCnqb9wxYW0=
-github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
-github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/spf13/pflag v1.0.6 h1:jFzHGLGAlb3ruxLB8MhbI6A8+AQX/2eW4qeyNZXNp2o=
 github.com/spf13/pflag v1.0.6/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
diff --git a/pkg/reference/reference.go b/pkg/reference/reference.go
index e77705d..82f54d0 100644
--- a/pkg/reference/reference.go
+++ b/pkg/reference/reference.go
@@ -20,6 +20,8 @@ package reference
 
 import (
 	"context"
+	"maps"
+	"slices"
 	"strconv"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -361,9 +363,10 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe
 		return MultiResolutionResponse{ResolvedValues: req.CurrentValues, ResolvedReferences: req.References}, nil
 	}
 
+	valueMap := make(map[string]xpv1.Reference)
+
 	// The references are already set - resolve them.
 	if len(req.References) > 0 {
-		vals := make([]string, len(req.References))
 		for i := range req.References {
 			if err := r.client.Get(ctx, types.NamespacedName{Name: req.References[i].Name}, req.To.Managed); err != nil {
 				if kerrors.IsNotFound(err) {
@@ -371,10 +374,12 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe
 				}
 				return MultiResolutionResponse{}, errors.Wrap(err, errGetManaged)
 			}
-			vals[i] = req.Extract(req.To.Managed)
+			valueMap[req.Extract(req.To.Managed)] = req.References[i]
 		}
 
-		rsp := MultiResolutionResponse{ResolvedValues: vals, ResolvedReferences: req.References}
+		sortedKeys, sortedRefs := sortMapByKeys(valueMap)
+
+		rsp := MultiResolutionResponse{ResolvedValues: sortedKeys, ResolvedReferences: sortedRefs}
 		return rsp, rsp.Validate()
 	}
 
@@ -383,19 +388,17 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe
 		return MultiResolutionResponse{}, errors.Wrap(err, errListManaged)
 	}
 
-	items := req.To.List.GetItems()
-	refs := make([]xpv1.Reference, 0, len(items))
-	vals := make([]string, 0, len(items))
 	for _, to := range req.To.List.GetItems() {
 		if ControllersMustMatch(req.Selector) && !meta.HaveSameController(r.from, to) {
 			continue
 		}
 
-		vals = append(vals, req.Extract(to))
-		refs = append(refs, xpv1.Reference{Name: to.GetName()})
+		valueMap[req.Extract(to)] = xpv1.Reference{Name: to.GetName()}
 	}
 
-	rsp := MultiResolutionResponse{ResolvedValues: vals, ResolvedReferences: refs}
+	sortedKeys, sortedRefs := sortMapByKeys(valueMap)
+
+	rsp := MultiResolutionResponse{ResolvedValues: sortedKeys, ResolvedReferences: sortedRefs}
 	return rsp, getResolutionError(req.Selector.Policy, rsp.Validate())
 }
 
@@ -406,6 +409,15 @@ func getResolutionError(p *xpv1.Policy, err error) error {
 	return nil
 }
 
+func sortMapByKeys(m map[string]xpv1.Reference) ([]string, []xpv1.Reference) {
+	keys := slices.Sorted(maps.Keys(m))
+	values := make([]xpv1.Reference, 0, len(keys))
+	for _, k := range keys {
+		values = append(values, m[k])
+	}
+	return keys, values
+}
+
 // ControllersMustMatch returns true if the supplied Selector requires that a
 // reference be to a managed resource whose controller reference matches the
 // referencing resource.



Test patch:
diff --git a/pkg/reference/reference_test.go b/pkg/reference/reference_test.go
index 6697d25..94ba268 100644
--- a/pkg/reference/reference_test.go
+++ b/pkg/reference/reference_test.go
@@ -458,6 +458,7 @@ func TestResolveMultiple(t *testing.T) {
 	errBoom := errors.New("boom")
 	now := metav1.Now()
 	value := "coolv"
+	value2 := "cooler"
 	ref := xpv1.Reference{Name: "cool"}
 	optionalPolicy := xpv1.ResolutionPolicyOptional
 	alwaysPolicy := xpv1.ResolvePolicyAlways
@@ -469,6 +470,11 @@ func TestResolveMultiple(t *testing.T) {
 	meta.SetExternalName(controlled, value)
 	meta.AddControllerReference(controlled, meta.AsController(&xpv1.TypedReference{UID: types.UID("very-unique")}))
 
+	controlled2 := &fake.Managed{}
+	controlled2.SetName(value2)
+	meta.SetExternalName(controlled2, value2)
+	meta.AddControllerReference(controlled2, meta.AsController(&xpv1.TypedReference{UID: types.UID("very-unique")}))
+
 	type args struct {
 		ctx context.Context
 		req MultiResolutionRequest
@@ -757,6 +763,36 @@ func TestResolveMultiple(t *testing.T) {
 				},
 			},
 		},
+		"OrderOutput": {
+			reason: "Output values should ordered",
+			c: &test.MockClient{
+				MockList: test.NewMockListFn(nil),
+			},
+			from: controlled,
+			args: args{
+				req: MultiResolutionRequest{
+					Selector: &xpv1.Selector{
+						MatchControllerRef: func() *bool { t := true; return &t }(),
+					},
+					To: To{List: &FakeManagedList{
+						Items: []resource.Managed{
+							&fake.Managed{}, // A resource that does not match.
+							controlled,      // A resource with a matching controller reference.
+							&fake.Managed{}, // A resource that does not match.
+							controlled2,     // A resource with a matching controller reference.
+						},
+					}},
+					Extract: ExternalName(),
+				},
+			},
+			want: want{
+				rsp: MultiResolutionResponse{
+					ResolvedValues:     []string{value2, value},
+					ResolvedReferences: []xpv1.Reference{{Name: value2}, {Name: value}},
+				},
+				err: nil,
+			},
+		},
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {