From 0dc6ab974a09eed585677efa11703f912ac40a69 Mon Sep 17 00:00:00 2001
From: raymondmiaochaoyue <raymondmiaochaoyue@didiglobal.com>
Date: Thu, 3 Nov 2022 18:17:29 +0800
Subject: [PATCH] make multi cluster ResourceVersion string stable

Signed-off-by: raymondmiaochaoyue <raymondmiaochaoyue@didiglobal.com>
---
 pkg/search/proxy/store/util.go      | 56 ++++++++++++++++++++++++++++-
 pkg/search/proxy/store/util_test.go | 48 +++++++++++++++++++++++--
 2 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/pkg/search/proxy/store/util.go b/pkg/search/proxy/store/util.go
index 1815639e8..d52d7fb0f 100644
--- a/pkg/search/proxy/store/util.go
+++ b/pkg/search/proxy/store/util.go
@@ -3,6 +3,7 @@ package store
 import (
 	"encoding/base64"
 	"encoding/json"
+	"sort"
 	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -63,13 +64,66 @@ func (m *multiClusterResourceVersion) String() string {
 	if m.isZero {
 		return "0"
 	}
+	// todo consider to separate this two scenarios:
+	// 1. client do not send ResourceVersion
+	// 2. client send ResourceVersion with empty cluster.
 	if len(m.rvs) == 0 {
 		return ""
 	}
-	buf, _ := json.Marshal(&m.rvs)
+	buf := marshalRvs(m.rvs)
 	return base64.RawURLEncoding.EncodeToString(buf)
 }
 
+func marshalRvs(rvs map[string]string) []byte {
+	// We must make sure the returned ResourceVersion string is stable, because client might use this string
+	// for equality comparison. But hashmap's key iteration order is not determined.
+	// So we can't use json encoding library directly.
+	// Instead, we convert the map to a slice, sort the slice by `Cluster` field, then manually build a string.
+	// The result string resembles json encoding result to keep compatible with older version of proxy.
+
+	if len(rvs) == 0 {
+		// need to keep sync with `func (m *multiClusterResourceVersion) String() string`
+		return nil
+	}
+
+	type onWireRvs struct {
+		Cluster         string
+		ResourceVersion string
+	}
+
+	slice := make([]onWireRvs, 0, len(rvs))
+
+	for clusterName, version := range rvs {
+		obj := onWireRvs{clusterName, version}
+		slice = append(slice, obj)
+	}
+
+	sort.Slice(slice, func(i, j int) bool {
+		return slice[i].Cluster < slice[j].Cluster
+	})
+
+	// Q: Why preallocate []byte with this capacity?
+	// A: Consider this json `{"cluster1":"1","cluster2":"2"}`
+	// For begin and end, need "{" and "}", this took 2 bytes.
+	// For `"cluster1":"1",`, there's 6 bytes. And we add 14bytes for longer cluster name and resource version.
+	var encoded = make([]byte, 0, (len(slice[0].Cluster)+len(slice[0].ResourceVersion)+20)*len(slice)+2)
+	encoded = append(encoded, '{')
+	for i, n := 0, len(slice); i < n; i++ {
+		encoded = append(encoded, '"')
+		encoded = append(encoded, slice[i].Cluster...)
+		encoded = append(encoded, `":"`...)
+		encoded = append(encoded, slice[i].ResourceVersion...)
+		encoded = append(encoded, '"')
+
+		if i != n-1 {
+			encoded = append(encoded, ',')
+		}
+	}
+	encoded = append(encoded, '}')
+
+	return encoded
+}
+
 type multiClusterContinue struct {
 	Cluster  string `json:"cluster,omitempty"`
 	Continue string `json:"continue,omitempty"`
diff --git a/pkg/search/proxy/store/util_test.go b/pkg/search/proxy/store/util_test.go
index 4da0998f5..6bd078004 100644
--- a/pkg/search/proxy/store/util_test.go
+++ b/pkg/search/proxy/store/util_test.go
@@ -201,7 +201,7 @@ func Test_newMultiClusterResourceVersionFromString(t *testing.T) {
 			},
 		},
 		{
-			name: "success",
+			name: "success - normal",
 			args: args{
 				s: base64.RawURLEncoding.EncodeToString([]byte(`{"cluster1":"1","cluster2":"2"}`)),
 			},
@@ -212,6 +212,30 @@ func Test_newMultiClusterResourceVersionFromString(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "success - empty cluster name",
+			args: args{
+				s: base64.RawURLEncoding.EncodeToString([]byte(`{"":"1","cluster2":"2"}`)),
+			},
+			want: &multiClusterResourceVersion{
+				rvs: map[string]string{
+					"":         "1",
+					"cluster2": "2",
+				},
+			},
+		},
+		{
+			name: "success - empty ResourceVersion",
+			args: args{
+				s: base64.RawURLEncoding.EncodeToString([]byte(`{"cluster1":"","cluster2":""}`)),
+			},
+			want: &multiClusterResourceVersion{
+				rvs: map[string]string{
+					"cluster1": "",
+					"cluster2": "",
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -310,7 +334,7 @@ func Test_multiClusterResourceVersion_String(t *testing.T) {
 			want: "",
 		},
 		{
-			name: "get success",
+			name: "get success - normal",
 			fields: fields{
 				rvs: map[string]string{
 					"cluster1": "1",
@@ -319,6 +343,26 @@ func Test_multiClusterResourceVersion_String(t *testing.T) {
 			},
 			want: base64.RawURLEncoding.EncodeToString([]byte(`{"cluster1":"1","cluster2":"2"}`)),
 		},
+		{
+			name: "get success - empty cluster name",
+			fields: fields{
+				rvs: map[string]string{
+					"":         "1",
+					"cluster2": "2",
+				},
+			},
+			want: base64.RawURLEncoding.EncodeToString([]byte(`{"":"1","cluster2":"2"}`)),
+		},
+		{
+			name: "get success - empty ResourceVersion",
+			fields: fields{
+				rvs: map[string]string{
+					"cluster1": "",
+					"cluster2": "",
+				},
+			},
+			want: base64.RawURLEncoding.EncodeToString([]byte(`{"cluster1":"","cluster2":""}`)),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {