From 7611fb75d76b4762f90e3cf0e4b2d584f8500751 Mon Sep 17 00:00:00 2001
From: Guilhem Lettron <glettron@akamai.com>
Date: Wed, 15 Jan 2025 14:31:18 +0100
Subject: [PATCH] fix: order values in MultiResolutionResponse

Signed-off-by: Guilhem Lettron <glettron@akamai.com>
---
 pkg/reference/reference.go      | 35 +++++++++++++++++++++++---------
 pkg/reference/reference_test.go | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/pkg/reference/reference.go b/pkg/reference/reference.go
index 3c36d906a..9c18a39ce 100644
--- a/pkg/reference/reference.go
+++ b/pkg/reference/reference.go
@@ -20,6 +20,7 @@ package reference
 
 import (
 	"context"
+	"slices"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
@@ -238,9 +239,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) {
@@ -248,10 +250,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()
 	}
 
@@ -260,19 +264,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())
 }
 
@@ -283,6 +285,21 @@ func getResolutionError(p *xpv1.Policy, err error) error {
 	return nil
 }
 
+func sortMapByKeys(m map[string]xpv1.Reference) ([]string, []xpv1.Reference) {
+	// TODO: use go 1.23 maps.Keys when available
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	slices.Sort(keys)
+
+	values := make([]xpv1.Reference, 0, len(m))
+	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.
diff --git a/pkg/reference/reference_test.go b/pkg/reference/reference_test.go
index 67106c4aa..1b812bf01 100644
--- a/pkg/reference/reference_test.go
+++ b/pkg/reference/reference_test.go
@@ -367,6 +367,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
@@ -378,6 +379,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
@@ -666,6 +672,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) {