From 93c09c1c85773d5ade114be801d5021648349250 Mon Sep 17 00:00:00 2001 From: JimDevil <709192853@qq.com> Date: Sat, 18 Jan 2025 18:55:25 +0800 Subject: [PATCH] fix: resourceregistry merge the same cluster gvk's namespace Signed-off-by: JimDevil <709192853@qq.com> --- pkg/search/proxy/controller.go | 41 ++++++---- pkg/search/proxy/controller_test.go | 112 ++++++++++++++++++++------- pkg/search/proxy/store/util.go | 14 ++++ pkg/search/proxy/store/util_test.go | 90 +++++++++++++++++++++ pkg/search/proxy/testing/constant.go | 7 +- 5 files changed, 219 insertions(+), 45 deletions(-) diff --git a/pkg/search/proxy/controller.go b/pkg/search/proxy/controller.go index ac54d9a8338f..9a0254ce4f00 100644 --- a/pkg/search/proxy/controller.go +++ b/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 } - - 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 - } + ctl.mergeResourcesByClusters(resourcesByClusters, cluster, matchedResources) } } 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 diff --git a/pkg/search/proxy/controller_test.go b/pkg/search/proxy/controller_test.go index 26cd0dc9b257..3e4da5f8d55b 100644 --- a/pkg/search/proxy/controller_test.go +++ b/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 { - sort.Strings(ss) - return strings.Join(ss, ",") + newMultiNs := func(namespaces ...string) *store.MultiNamespace { + multiNs := store.NewMultiNamespace() + 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{ - "cluster1": echoStrings("pods", "nodes"), - "cluster2": echoStrings("pods", "nodes"), + want: map[string]map[string]*store.MultiNamespace{ + "cluster1": { + "pods": newMultiNs(), + "nodes": newMultiNs(), + }, + "cluster2": { + "pods": newMultiNs(), + "nodes": newMultiNs(), + }, }, }, { @@ -178,9 +189,13 @@ func TestController_reconcile(t *testing.T) { }, }, }, - want: map[string]string{ - "cluster1": echoStrings("pods"), - "cluster2": echoStrings("nodes"), + want: map[string]map[string]*store.MultiNamespace{ + "cluster1": { + "pods": newMultiNs(), + }, + "cluster2": { + "nodes": newMultiNs(), + }, }, }, { @@ -203,9 +218,14 @@ func TestController_reconcile(t *testing.T) { }, }, }, - want: map[string]string{ - "cluster1": echoStrings("pods", "nodes"), - "cluster2": echoStrings("nodes"), + want: map[string]map[string]*store.MultiNamespace{ + "cluster1": { + "pods": newMultiNs(), + "nodes": newMultiNs(), + }, + "cluster2": { + "nodes": newMultiNs(), + }, }, }, { @@ -226,8 +246,10 @@ func TestController_reconcile(t *testing.T) { }, }, }, - want: map[string]string{ - "cluster1": echoStrings("pods"), + want: map[string]map[string]*store.MultiNamespace{ + "cluster1": { + "pods": newMultiNs(), + }, }, }, { @@ -258,8 +280,10 @@ func TestController_reconcile(t *testing.T) { }, }, }, - want: map[string]string{ - "cluster1": echoStrings("pods"), + want: map[string]map[string]*store.MultiNamespace{ + "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)) - for resource := range resources { - resourceNames = append(resourceNames, resource.Resource) + resourceCaches := map[string]*store.MultiNamespace{} + for resource, multiNs := range resources { + resourceCaches[resource.Resource] = multiNs } - actual[clusterName] = echoStrings(resourceNames...) + actual[clusterName] = resourceCaches } if len(actual) != len(m) { return fmt.Errorf("cluster duplicate: %#v", m) diff --git a/pkg/search/proxy/store/util.go b/pkg/search/proxy/store/util.go index ed7c51cf30e6..88e73a7186e8 100644 --- a/pkg/search/proxy/store/util.go +++ b/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{ diff --git a/pkg/search/proxy/store/util_test.go b/pkg/search/proxy/store/util_test.go index 20981c49a837..8317ee686acc 100644 --- a/pkg/search/proxy/store/util_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/search/proxy/testing/constant.go b/pkg/search/proxy/testing/constant.go index b4c6f3f9ad33..eb5bb605fde8 100644 --- a/pkg/search/proxy/testing/constant.go +++ b/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