Merge pull request #6065 from JimDevil/feat-search

fix(search/proxy/controller): resourceregistry merge ns

pkg/search/proxy/controller.go

@@ -228,29 +228,36 @@ func (ctl *Controller) reconcile(util.QueueKey) error {
 				klog.Warningf("cluster %s is notReady", cluster.Name)
 				continue
 			}
-
+			ctl.mergeResourcesByClusters(resourcesByClusters, cluster, matchedResources)
-			if _, exist := resourcesByClusters[cluster.Name]; !exist {
-				resourcesByClusters[cluster.Name] = make(map[schema.GroupVersionResource]*store.MultiNamespace)
-			}
-
-			for resource, multiNS := range matchedResources {
-				gvk, err := ctl.restMapper.KindFor(resource)
-				if err != nil {
-					klog.Errorf("Failed to get gvk: %v", err)
-					continue
-				}
-				if !helper.IsAPIEnabled(cluster.Status.APIEnablements, gvk.GroupVersion().String(), gvk.Kind) {
-					klog.Warningf("Resource %s is not enabled for cluster %s", resource.String(), cluster)
-					continue
-				}
-				resourcesByClusters[cluster.Name][resource] = multiNS
-			}
 		}
 	}
 
 	return ctl.store.UpdateCache(resourcesByClusters, registeredResources)
 }
 
+func (ctl *Controller) mergeResourcesByClusters(resourcesByClusters map[string]map[schema.GroupVersionResource]*store.MultiNamespace, cluster *clusterv1alpha1.Cluster, matchedResources map[schema.GroupVersionResource]*store.MultiNamespace) {
+	if _, exist := resourcesByClusters[cluster.Name]; !exist {
+		resourcesByClusters[cluster.Name] = make(map[schema.GroupVersionResource]*store.MultiNamespace)
+	}
+
+	for resource, multiNS := range matchedResources {
+		gvk, err := ctl.restMapper.KindFor(resource)
+		if err != nil {
+			klog.Errorf("Failed to get gvk: %v", err)
+			continue
+		}
+		if !helper.IsAPIEnabled(cluster.Status.APIEnablements, gvk.GroupVersion().String(), gvk.Kind) {
+			klog.Warningf("Resource %s is not enabled for cluster %s", resource.String(), cluster)
+			continue
+		}
+		if ns, exist := resourcesByClusters[cluster.Name][resource]; !exist {
+			resourcesByClusters[cluster.Name][resource] = multiNS
+		} else {
+			resourcesByClusters[cluster.Name][resource] = ns.Merge(multiNS)
+		}
+	}
+}
+
 type errorHTTPHandler struct {
 	requestInfo          *request.RequestInfo
 	err                  error

pkg/search/proxy/controller_test.go

@@ -22,8 +22,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"reflect"
-	"sort"
-	"strings"
 	"testing"
 	"time"
 
@@ -104,19 +102,26 @@ func TestController(t *testing.T) {
 }
 
 func TestController_reconcile(t *testing.T) {
-	echoStrings := func(ss ...string) string {
+	newMultiNs := func(namespaces ...string) *store.MultiNamespace {
-		sort.Strings(ss)
+		multiNs := store.NewMultiNamespace()
-		return strings.Join(ss, ",")
+		if len(namespaces) == 0 {
+			multiNs.Add(metav1.NamespaceAll)
+			return multiNs
+		}
+		for _, ns := range namespaces {
+			multiNs.Add(ns)
+		}
+		return multiNs
 	}
 	tests := []struct {
 		name  string
 		input []runtime.Object
-		want  map[string]string
+		want  map[string]map[string]*store.MultiNamespace
 	}{
 		{
 			name:  "all empty",
 			input: []runtime.Object{},
-			want:  map[string]string{},
+			want:  map[string]map[string]*store.MultiNamespace{},
 		},
 		{
 			name: "resource registered, while cluster not registered",
@@ -133,7 +138,7 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{},
+			want: map[string]map[string]*store.MultiNamespace{},
 		},
 		{
 			name: "pod and node are registered",
@@ -153,9 +158,15 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{
+			want: map[string]map[string]*store.MultiNamespace{
-				"cluster1": echoStrings("pods", "nodes"),
+				"cluster1": {
-				"cluster2": echoStrings("pods", "nodes"),
+					"pods":  newMultiNs(),
+					"nodes": newMultiNs(),
+				},
+				"cluster2": {
+					"pods":  newMultiNs(),
+					"nodes": newMultiNs(),
+				},
 			},
 		},
 		{
@@ -178,9 +189,13 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{
+			want: map[string]map[string]*store.MultiNamespace{
-				"cluster1": echoStrings("pods"),
+				"cluster1": {
-				"cluster2": echoStrings("nodes"),
+					"pods": newMultiNs(),
+				},
+				"cluster2": {
+					"nodes": newMultiNs(),
+				},
 			},
 		},
 		{
@@ -203,9 +218,14 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{
+			want: map[string]map[string]*store.MultiNamespace{
-				"cluster1": echoStrings("pods", "nodes"),
+				"cluster1": {
-				"cluster2": echoStrings("nodes"),
+					"pods":  newMultiNs(),
+					"nodes": newMultiNs(),
+				},
+				"cluster2": {
+					"nodes": newMultiNs(),
+				},
 			},
 		},
 		{
@@ -226,8 +246,10 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{
+			want: map[string]map[string]*store.MultiNamespace{
-				"cluster1": echoStrings("pods"),
+				"cluster1": {
+					"pods": newMultiNs(),
+				},
 			},
 		},
 		{
@@ -258,8 +280,10 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{
+			want: map[string]map[string]*store.MultiNamespace{
-				"cluster1": echoStrings("pods"),
+				"cluster1": {
+					"pods": newMultiNs(),
+				},
 			},
 		},
 		{
@@ -278,13 +302,47 @@ func TestController_reconcile(t *testing.T) {
 					},
 				},
 			},
-			want: map[string]string{},
+			want: map[string]map[string]*store.MultiNamespace{},
+		},
+		{
+			name: "register pod twice in two ResourceRegistries with different namespace",
+			input: []runtime.Object{
+				newCluster("cluster1"),
+				newCluster("cluster2"),
+				&searchv1alpha1.ResourceRegistry{
+					ObjectMeta: metav1.ObjectMeta{Name: "rr1"},
+					Spec: searchv1alpha1.ResourceRegistrySpec{
+						TargetCluster: policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster1"},
+						},
+						ResourceSelectors: []searchv1alpha1.ResourceSelector{
+							proxytest.PodSelectorWithNS1,
+						},
+					},
+				},
+				&searchv1alpha1.ResourceRegistry{
+					ObjectMeta: metav1.ObjectMeta{Name: "rr2"},
+					Spec: searchv1alpha1.ResourceRegistrySpec{
+						TargetCluster: policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster1"},
+						},
+						ResourceSelectors: []searchv1alpha1.ResourceSelector{
+							proxytest.PodSelectorWithNS2,
+						},
+					},
+				},
+			},
+			want: map[string]map[string]*store.MultiNamespace{
+				"cluster1": {
+					"pods": newMultiNs("ns1", "ns2"),
+				},
+			},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			actual := map[string]string{}
+			actual := map[string]map[string]*store.MultiNamespace{}
 			karmadaClientset := karmadafake.NewSimpleClientset(tt.input...)
 			karmadaFactory := karmadainformers.NewSharedInformerFactory(karmadaClientset, 0)
 
@@ -295,11 +353,11 @@ func TestController_reconcile(t *testing.T) {
 				store: &proxytest.MockStore{
 					UpdateCacheFunc: func(m map[string]map[schema.GroupVersionResource]*store.MultiNamespace, _ map[schema.GroupVersionResource]struct{}) error {
 						for clusterName, resources := range m {
-							resourceNames := make([]string, 0, len(resources))
+							resourceCaches := map[string]*store.MultiNamespace{}
-							for resource := range resources {
+							for resource, multiNs := range resources {
-								resourceNames = append(resourceNames, resource.Resource)
+								resourceCaches[resource.Resource] = multiNs
 							}
-							actual[clusterName] = echoStrings(resourceNames...)
+							actual[clusterName] = resourceCaches
 						}
 						if len(actual) != len(m) {
 							return fmt.Errorf("cluster duplicate: %#v", m)

pkg/search/proxy/store/util.go

@@ -284,6 +284,20 @@ type MultiNamespace struct {
 	namespaces    sets.Set[string]
 }
 
+// Merge merges multiNS into n.
+func (n *MultiNamespace) Merge(multiNS *MultiNamespace) *MultiNamespace {
+	if n.allNamespaces || multiNS.allNamespaces {
+		n.allNamespaces = true
+		n.namespaces = nil
+		return n
+	}
+	if n.Equal(multiNS) {
+		return n
+	}
+	n.namespaces.Insert(multiNS.namespaces.Clone().UnsortedList()...)
+	return n
+}
+
 // NewMultiNamespace return a new empty MultiNamespace.
 func NewMultiNamespace() *MultiNamespace {
 	return &MultiNamespace{

pkg/search/proxy/store/util_test.go

@@ -1082,3 +1082,93 @@ func TestMultiNamespace_Equal(t *testing.T) {
 		})
 	}
 }
+
+func TestMultiNamespace_Merge(t *testing.T) {
+	type fields struct {
+		allNamespaces bool
+		namespaces    sets.Set[string]
+	}
+	type args struct {
+		multiNS *MultiNamespace
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		args   args
+		want   *MultiNamespace
+	}{
+		{
+			name: "all ns merge all ns",
+			fields: fields{
+				allNamespaces: true,
+			},
+			args: args{
+				multiNS: &MultiNamespace{
+					allNamespaces: true,
+				},
+			},
+			want: &MultiNamespace{
+				allNamespaces: true,
+			},
+		},
+		{
+			name: "all ns merge not all",
+			fields: fields{
+				allNamespaces: true,
+			},
+			args: args{
+				multiNS: &MultiNamespace{
+					allNamespaces: false,
+					namespaces:    sets.New[string]("foo"),
+				},
+			},
+			want: &MultiNamespace{
+				allNamespaces: true,
+			},
+		},
+		{
+			name: "not all ns merge all",
+			fields: fields{
+				allNamespaces: false,
+				namespaces:    sets.New[string]("foo"),
+			},
+			args: args{
+				multiNS: &MultiNamespace{
+					allNamespaces: true,
+				},
+			},
+			want: &MultiNamespace{
+				allNamespaces: true,
+			},
+		},
+		{
+			name: "not all ns merge not all ns",
+			fields: fields{
+				allNamespaces: false,
+				namespaces:    sets.New[string]("foo", "zoo"),
+			},
+			args: args{
+				multiNS: &MultiNamespace{
+					allNamespaces: false,
+					namespaces:    sets.New[string]("bar"),
+				},
+			},
+			want: &MultiNamespace{
+				allNamespaces: false,
+				namespaces:    sets.New[string]("foo", "bar", "zoo"),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			n := &MultiNamespace{
+				allNamespaces: tt.fields.allNamespaces,
+				namespaces:    tt.fields.namespaces,
+			}
+			got := n.Merge(tt.args.multiNS)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("MultiNamespace.Merge() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/search/proxy/testing/constant.go

@@ -35,7 +35,12 @@ var (
 	SecretGVR  = corev1.SchemeGroupVersion.WithResource("secret")
 	ClusterGVR = clusterv1alpha1.SchemeGroupVersion.WithResource("cluster")
 
-	PodSelector  = searchv1alpha1.ResourceSelector{APIVersion: PodGVK.GroupVersion().String(), Kind: PodGVK.Kind}
+	PodSelector = searchv1alpha1.ResourceSelector{APIVersion: PodGVK.GroupVersion().String(), Kind: PodGVK.Kind}
+
+	PodSelectorWithNS1 = searchv1alpha1.ResourceSelector{APIVersion: PodGVK.GroupVersion().String(), Kind: PodGVK.Kind, Namespace: "ns1"}
+
+	PodSelectorWithNS2 = searchv1alpha1.ResourceSelector{APIVersion: PodGVK.GroupVersion().String(), Kind: PodGVK.Kind, Namespace: "ns2"}
+
 	NodeSelector = searchv1alpha1.ResourceSelector{APIVersion: NodeGVK.GroupVersion().String(), Kind: NodeGVK.Kind}
 
 	RestMapper *meta.DefaultRESTMapper