From c943a3b9f32975bfcc25ff370baf3169d53babe6 Mon Sep 17 00:00:00 2001 From: Yan Zhu Date: Mon, 25 Sep 2023 19:47:41 +0800 Subject: [PATCH] client: client.MatchingFields support multiple indexes add support for multiple indexes when using client.MatchingFields --- pkg/cache/cache_test.go | 49 +++++++++++++++++++- pkg/cache/internal/cache_reader.go | 55 +++++++++++++++++++++-- pkg/client/example_test.go | 16 ++++++- pkg/client/fake/client.go | 23 ++++++---- pkg/client/fake/client_test.go | 7 +-- pkg/client/options.go | 2 +- pkg/internal/field/selector/utils.go | 16 ++++--- pkg/internal/field/selector/utils_test.go | 46 ++++--------------- 8 files changed, 149 insertions(+), 65 deletions(-) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index bff0c87083..232d8941fa 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -1941,13 +1941,13 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca Expect(err).NotTo(HaveOccurred()) By("indexing the Namespace objects with fixed values before starting") - pod := &corev1.Namespace{} + ns := &corev1.Namespace{} indexerValues := []string{"a", "b", "c"} fieldName := "fixedValues" indexFunc := func(obj client.Object) []string { return indexerValues } - Expect(informer.IndexField(context.TODO(), pod, fieldName, indexFunc)).To(Succeed()) + Expect(informer.IndexField(context.TODO(), ns, fieldName, indexFunc)).To(Succeed()) By("running the cache and waiting for it to sync") go func() { @@ -1968,6 +1968,51 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca Expect(indexerValues[1]).To(Equal("b")) Expect(indexerValues[2]).To(Equal("c")) }) + + It("should be able to matching fields with multiple indexes", func() { + By("creating the cache") + informer, err := cache.New(cfg, cache.Options{}) + Expect(err).NotTo(HaveOccurred()) + + pod := &corev1.Pod{} + By("indexing pods with label before starting") + fieldName1 := "indexByLabel" + indexFunc1 := func(obj client.Object) []string { + return []string{obj.(*corev1.Pod).Labels["common-label"]} + } + Expect(informer.IndexField(context.TODO(), pod, fieldName1, indexFunc1)).To(Succeed()) + By("indexing pods with restart policy before starting") + fieldName2 := "indexByPolicy" + indexFunc2 := func(obj client.Object) []string { + return []string{string(obj.(*corev1.Pod).Spec.RestartPolicy)} + } + Expect(informer.IndexField(context.TODO(), pod, fieldName2, indexFunc2)).To(Succeed()) + + By("running the cache and waiting for it to sync") + go func() { + defer GinkgoRecover() + Expect(informer.Start(informerCacheCtx)).To(Succeed()) + }() + Expect(informer.WaitForCacheSync(informerCacheCtx)).To(BeTrue()) + + By("listing pods with label index") + listObj := &corev1.PodList{} + Expect(informer.List(context.Background(), listObj, + client.MatchingFields{fieldName1: "common"})).To(Succeed()) + Expect(listObj.Items).To(HaveLen(2)) + + By("listing pods with restart policy index") + listObj = &corev1.PodList{} + Expect(informer.List(context.Background(), listObj, + client.MatchingFields{fieldName2: string(corev1.RestartPolicyNever)})).To(Succeed()) + Expect(listObj.Items).To(HaveLen(3)) + + By("listing pods with both fixed indexers 1 and 2") + listObj = &corev1.PodList{} + Expect(informer.List(context.Background(), listObj, + client.MatchingFields{fieldName1: "common", fieldName2: string(corev1.RestartPolicyNever)})).To(Succeed()) + Expect(listObj.Items).To(HaveLen(1)) + }) }) Context("with unstructured objects", func() { It("should be able to get informer for the object", func() { diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go index eb941f034e..2e4f5ce527 100644 --- a/pkg/cache/internal/cache_reader.go +++ b/pkg/cache/internal/cache_reader.go @@ -23,6 +23,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -117,16 +118,14 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli switch { case listOpts.FieldSelector != nil: - // TODO(directxman12): support more complicated field selectors by - // combining multiple indices, GetIndexers, etc - field, val, requiresExact := selector.RequiresExactMatch(listOpts.FieldSelector) + requiresExact := selector.RequiresExactMatch(listOpts.FieldSelector) if !requiresExact { return fmt.Errorf("non-exact field matches are not supported by the cache") } // list all objects by the field selector. If this is namespaced and we have one, ask for the // namespaced index key. Otherwise, ask for the non-namespaced variant by using the fake "all namespaces" // namespace. - objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(listOpts.Namespace, val)) + objs, err = byIndexes(c.indexer, listOpts.FieldSelector.Requirements(), listOpts.Namespace) case listOpts.Namespace != "": objs, err = c.indexer.ByIndex(cache.NamespaceIndex, listOpts.Namespace) default: @@ -178,6 +177,54 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli return apimeta.SetList(out, runtimeObjs) } +func byIndexes(indexer cache.Indexer, requires fields.Requirements, namespace string) ([]interface{}, error) { + var ( + err error + objs []interface{} + vals []string + ) + indexers := indexer.GetIndexers() + for idx, req := range requires { + indexName := FieldIndexName(req.Field) + indexedValue := KeyToNamespacedKey(namespace, req.Value) + if idx == 0 { + // we use first require to get snapshot data + // TODO(halfcrazy): use complicated index when client-go provides byIndexes + // https://github.com/kubernetes/kubernetes/issues/109329 + objs, err = indexer.ByIndex(indexName, indexedValue) + if err != nil { + return nil, err + } + if len(objs) == 0 { + return nil, nil + } + continue + } + fn, exist := indexers[indexName] + if !exist { + return nil, fmt.Errorf("index with name %s does not exist", indexName) + } + filteredObjects := make([]interface{}, 0, len(objs)) + for _, obj := range objs { + vals, err = fn(obj) + if err != nil { + return nil, err + } + for _, val := range vals { + if val == indexedValue { + filteredObjects = append(filteredObjects, obj) + break + } + } + } + if len(filteredObjects) == 0 { + return nil, nil + } + objs = filteredObjects + } + return objs, nil +} + // objectKeyToStorageKey converts an object key to store key. // It's akin to MetaNamespaceKeyFunc. It's separate from // String to allow keeping the key format easily in sync with diff --git a/pkg/client/example_test.go b/pkg/client/example_test.go index 7d4cb8c616..db002a4c69 100644 --- a/pkg/client/example_test.go +++ b/pkg/client/example_test.go @@ -247,7 +247,7 @@ func ExampleClient_deleteAllOf() { } // This example shows how to set up and consume a field selector over a pod's volumes' secretName field. -func ExampleFieldIndexer_secretName() { +func ExampleFieldIndexer_secretNameNode() { // someIndexer is a FieldIndexer over a Cache _ = someIndexer.IndexField(context.TODO(), &corev1.Pod{}, "spec.volumes.secret.secretName", func(o client.Object) []string { var res []string @@ -261,8 +261,20 @@ func ExampleFieldIndexer_secretName() { return res }) + _ = someIndexer.IndexField(context.TODO(), &corev1.Pod{}, "spec.NodeName", func(o client.Object) []string { + nodeName := o.(*corev1.Pod).Spec.NodeName + if nodeName != "" { + return []string{nodeName} + } + return nil + }) + // elsewhere (e.g. in your reconciler) mySecretName := "someSecret" // derived from the reconcile.Request, for instance + myNode := "master-0" var podsWithSecrets corev1.PodList - _ = c.List(context.Background(), &podsWithSecrets, client.MatchingFields{"spec.volumes.secret.secretName": mySecretName}) + _ = c.List(context.Background(), &podsWithSecrets, client.MatchingFields{ + "spec.volumes.secret.secretName": mySecretName, + "spec.NodeName": myNode, + }) } diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index b3819926c0..71a0654cf9 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -586,9 +586,7 @@ func (c *fakeClient) filterList(list []runtime.Object, gvk schema.GroupVersionKi } func (c *fakeClient) filterWithFields(list []runtime.Object, gvk schema.GroupVersionKind, fs fields.Selector) ([]runtime.Object, error) { - // We only allow filtering on the basis of a single field to ensure consistency with the - // behavior of the cache reader (which we're faking here). - fieldKey, fieldVal, requiresExact := selector.RequiresExactMatch(fs) + requiresExact := selector.RequiresExactMatch(fs) if !requiresExact { return nil, fmt.Errorf("field selector %s is not in one of the two supported forms \"key==val\" or \"key=val\"", fs) @@ -597,15 +595,24 @@ func (c *fakeClient) filterWithFields(list []runtime.Object, gvk schema.GroupVer // Field selection is mimicked via indexes, so there's no sane answer this function can give // if there are no indexes registered for the GroupVersionKind of the objects in the list. indexes := c.indexes[gvk] - if len(indexes) == 0 || indexes[fieldKey] == nil { - return nil, fmt.Errorf("List on GroupVersionKind %v specifies selector on field %s, but no "+ - "index with name %s has been registered for GroupVersionKind %v", gvk, fieldKey, fieldKey, gvk) + for _, req := range fs.Requirements() { + if len(indexes) == 0 || indexes[req.Field] == nil { + return nil, fmt.Errorf("List on GroupVersionKind %v specifies selector on field %s, but no "+ + "index with name %s has been registered for GroupVersionKind %v", gvk, req.Field, req.Field, gvk) + } } - indexExtractor := indexes[fieldKey] filteredList := make([]runtime.Object, 0, len(list)) for _, obj := range list { - if c.objMatchesFieldSelector(obj, indexExtractor, fieldVal) { + matches := true + for _, req := range fs.Requirements() { + indexExtractor := indexes[req.Field] + if !c.objMatchesFieldSelector(obj, indexExtractor, req.Value) { + matches = false + break + } + } + if matches { filteredList = append(filteredList, obj) } } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 810cf5499c..2cec470e49 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1333,14 +1333,15 @@ var _ = Describe("Fake client", func() { Expect(list.Items).To(BeEmpty()) }) - It("errors when field selector uses two requirements", func() { + It("no error when field selector uses two requirements", func() { listOpts := &client.ListOptions{ FieldSelector: fields.AndSelectors( fields.OneTermEqualSelector("spec.replicas", "1"), fields.OneTermEqualSelector("spec.strategy.type", string(appsv1.RecreateDeploymentStrategyType)), )} - err := cl.List(context.Background(), &appsv1.DeploymentList{}, listOpts) - Expect(err).To(HaveOccurred()) + list := &appsv1.DeploymentList{} + Expect(cl.List(context.Background(), list, listOpts)).To(Succeed()) + Expect(list.Items).To(ConsistOf(*dep)) }) }) }) diff --git a/pkg/client/options.go b/pkg/client/options.go index d81bf25de9..9c27309bc9 100644 --- a/pkg/client/options.go +++ b/pkg/client/options.go @@ -419,7 +419,7 @@ type ListOptions struct { LabelSelector labels.Selector // FieldSelector filters results by a particular field. In order // to use this with cache-based implementations, restrict usage to - // a single field-value pair that's been added to the indexers. + // exact match field-value pair that's been added to the indexers. FieldSelector fields.Selector // Namespace represents the namespace to list for, or empty for diff --git a/pkg/internal/field/selector/utils.go b/pkg/internal/field/selector/utils.go index 4f6d084318..8f6dc71ede 100644 --- a/pkg/internal/field/selector/utils.go +++ b/pkg/internal/field/selector/utils.go @@ -22,14 +22,16 @@ import ( ) // RequiresExactMatch checks if the given field selector is of the form `k=v` or `k==v`. -func RequiresExactMatch(sel fields.Selector) (field, val string, required bool) { +func RequiresExactMatch(sel fields.Selector) bool { reqs := sel.Requirements() - if len(reqs) != 1 { - return "", "", false + if len(reqs) == 0 { + return false } - req := reqs[0] - if req.Operator != selection.Equals && req.Operator != selection.DoubleEquals { - return "", "", false + + for _, req := range reqs { + if req.Operator != selection.Equals && req.Operator != selection.DoubleEquals { + return false + } } - return req.Field, req.Value, true + return true } diff --git a/pkg/internal/field/selector/utils_test.go b/pkg/internal/field/selector/utils_test.go index fba214ff16..a48bbf4e5a 100644 --- a/pkg/internal/field/selector/utils_test.go +++ b/pkg/internal/field/selector/utils_test.go @@ -27,62 +27,32 @@ import ( var _ = Describe("RequiresExactMatch function", func() { It("Returns false when the selector matches everything", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.Everything()) + requiresExactMatch := RequiresExactMatch(fields.Everything()) Expect(requiresExactMatch).To(BeFalse()) }) It("Returns false when the selector matches nothing", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.Nothing()) + requiresExactMatch := RequiresExactMatch(fields.Nothing()) Expect(requiresExactMatch).To(BeFalse()) }) It("Returns false when the selector has the form key!=val", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key!=val")) + requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key!=val")) Expect(requiresExactMatch).To(BeFalse()) }) - It("Returns false when the selector has the form key1==val1,key2==val2", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key1==val1,key2==val2")) - Expect(requiresExactMatch).To(BeFalse()) + It("Returns true when the selector has the form key1==val1,key2==val2", func() { + requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key1==val1,key2==val2")) + Expect(requiresExactMatch).To(BeTrue()) }) It("Returns true when the selector has the form key==val", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key==val")) + requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key==val")) Expect(requiresExactMatch).To(BeTrue()) }) It("Returns true when the selector has the form key=val", func() { - _, _, requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key=val")) + requiresExactMatch := RequiresExactMatch(fields.ParseSelectorOrDie("key=val")) Expect(requiresExactMatch).To(BeTrue()) }) - - It("Returns empty key and value when the selector matches everything", func() { - key, val, _ := RequiresExactMatch(fields.Everything()) - Expect(key).To(Equal("")) - Expect(val).To(Equal("")) - }) - - It("Returns empty key and value when the selector matches nothing", func() { - key, val, _ := RequiresExactMatch(fields.Nothing()) - Expect(key).To(Equal("")) - Expect(val).To(Equal("")) - }) - - It("Returns empty key and value when the selector has the form key!=val", func() { - key, val, _ := RequiresExactMatch(fields.ParseSelectorOrDie("key!=val")) - Expect(key).To(Equal("")) - Expect(val).To(Equal("")) - }) - - It("Returns key and value when the selector has the form key==val", func() { - key, val, _ := RequiresExactMatch(fields.ParseSelectorOrDie("key==val")) - Expect(key).To(Equal("key")) - Expect(val).To(Equal("val")) - }) - - It("Returns key and value when the selector has the form key=val", func() { - key, val, _ := RequiresExactMatch(fields.ParseSelectorOrDie("key=val")) - Expect(key).To(Equal("key")) - Expect(val).To(Equal("val")) - }) })