Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add indexes to fake client to mimic field selectors #2025

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions pkg/cache/internal/cache_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@ 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"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/client-go/tools/cache"
"sigs.k8s.io/controller-runtime/pkg/internal/field/selector"

"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -116,7 +115,7 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli
case listOpts.FieldSelector != nil:
// TODO(directxman12): support more complicated field selectors by
// combining multiple indices, GetIndexers, etc
field, val, requiresExact := requiresExactMatch(listOpts.FieldSelector)
field, val, requiresExact := selector.RequiresExactMatch(listOpts.FieldSelector)
if !requiresExact {
return fmt.Errorf("non-exact field matches are not supported by the cache")
}
Expand Down Expand Up @@ -186,19 +185,6 @@ func objectKeyToStoreKey(k client.ObjectKey) string {
return k.Namespace + "/" + k.Name
}

// 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) {
reqs := sel.Requirements()
if len(reqs) != 1 {
return "", "", false
}
req := reqs[0]
if req.Operator != selection.Equals && req.Operator != selection.DoubleEquals {
return "", "", false
}
return req.Field, req.Value, true
}

// FieldIndexName constructs the name of the index over the given field,
// for use with an indexer.
func FieldIndexName(field string) string {
Expand Down
133 changes: 121 additions & 12 deletions pkg/client/fake/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
utilrand "k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/testing"
"sigs.k8s.io/controller-runtime/pkg/internal/field/selector"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
Expand All @@ -49,9 +52,14 @@ type versionedTracker struct {
}

type fakeClient struct {
tracker versionedTracker
scheme *runtime.Scheme
restMapper meta.RESTMapper
tracker versionedTracker
scheme *runtime.Scheme
restMapper meta.RESTMapper

// indexes maps each GroupVersionResource (GVR) to the indexes registered for that GVR.
// The inner map maps from index name to IndexerFunc.
indexes map[schema.GroupVersionResource]map[string]client.IndexerFunc

schemeWriteLock sync.Mutex
}

Expand Down Expand Up @@ -93,6 +101,10 @@ type ClientBuilder struct {
initLists []client.ObjectList
initRuntimeObjects []runtime.Object
objectTracker testing.ObjectTracker

// indexes maps each GroupVersionResource (GVR) to the indexes registered for that GVR.
// The inner map maps from index name to IndexerFunc.
indexes map[schema.GroupVersionResource]map[string]client.IndexerFunc
}

// WithScheme sets this builder's internal scheme.
Expand Down Expand Up @@ -135,6 +147,31 @@ func (f *ClientBuilder) WithObjectTracker(ot testing.ObjectTracker) *ClientBuild
return f
}

// WithIndex can be optionally used to register an index with name `name` and indexer `indexer` for
// API objects of GroupVersionResource `gvr` in the fake client.
// It can be invoked multiple times, both with different GroupVersionResource or the same one.
// Invoking WithIndex twice with the same `name` and `gvr` will panic.
func (f *ClientBuilder) WithIndex(gvr schema.GroupVersionResource, name string, indexer client.IndexerFunc) *ClientBuilder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than taking an gvr, could we take a client.Object instead? And maybe stick with the argument naming used in the FieldIndexer (i.E. obj Object, field string, extractValue IndexerFunc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5db9d19

// If this is the first index being registered, we initialize the map storing all the indexes.
if f.indexes == nil {
f.indexes = make(map[schema.GroupVersionResource]map[string]client.IndexerFunc)
}

// If this is the first index being registered for the input GroupVersionResource, we initialize
// the map storing the indexes for that GroupVersionResource.
if f.indexes[gvr] == nil {
f.indexes[gvr] = make(map[string]client.IndexerFunc)
}

if _, nameAlreadyTaken := f.indexes[gvr][name]; nameAlreadyTaken {
panic(fmt.Errorf("indexer conflict: index name %s is already registered for GroupVersionResource %v", name, gvr))
}

f.indexes[gvr][name] = indexer

return f
}

// Build builds and returns a new fake client.
func (f *ClientBuilder) Build() client.WithWatch {
if f.scheme == nil {
Expand Down Expand Up @@ -171,6 +208,7 @@ func (f *ClientBuilder) Build() client.WithWatch {
tracker: tracker,
scheme: f.scheme,
restMapper: f.restMapper,
indexes: f.indexes,
}
}

Expand Down Expand Up @@ -420,21 +458,92 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
return err
}

if listOpts.LabelSelector != nil {
objs, err := meta.ExtractList(obj)
if listOpts.LabelSelector == nil && listOpts.FieldSelector == nil {
return nil
}

// If we're here, either a label or field selector are specified (or both), so before we return
// the list we must filter it. If both selectors are set, they are ANDed.
objs, err := meta.ExtractList(obj)
if err != nil {
return err
}

filteredList, err := c.filterList(objs, gvr, listOpts.LabelSelector, listOpts.FieldSelector)
if err != nil {
return err
}

return meta.SetList(obj, filteredList)
}

func (c *fakeClient) filterList(list []runtime.Object, gvr schema.GroupVersionResource, ls labels.Selector, fs fields.Selector) ([]runtime.Object, error) {
// Filter the objects with the label selector
filteredList := list
if ls != nil {
objsFilteredByLabel, err := objectutil.FilterWithLabels(list, ls)
if err != nil {
return err
return nil, err
}
filteredObjs, err := objectutil.FilterWithLabels(objs, listOpts.LabelSelector)
filteredList = objsFilteredByLabel
}

// Filter the result of the previous pass with the field selector
if fs != nil {
objsFilteredByField, err := c.filterWithFields(filteredList, gvr, fs)
if err != nil {
return err
return nil, err
}
err = meta.SetList(obj, filteredObjs)
if err != nil {
return err
filteredList = objsFilteredByField
}

return filteredList, nil
}

func (c *fakeClient) filterWithFields(list []runtime.Object, gvr schema.GroupVersionResource, 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)
if !requiresExact {
return nil, fmt.Errorf("field selector %s is not in one of the two supported forms \"key==val\" or \"key=val\"",
fs)
}

// Field selection is mimicked via indexes, so there's no sane answer this function can give
// if there are no indexes registered for the GroupVersionResource of the objects in the list.
indexes, listGVRHasIndexes := c.indexes[gvr]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return the same error for this and the second error condition, for the user of this it is the same mistake so getting one or the other might be confusing because what they have to change is the same in both cases

Suggested change
indexes, listGVRHasIndexes := c.indexes[gvr]
if indexes := c.indexes[gvr]; indexers == nil || indexers[fieldKey] == nil {
return nil, fmt.Errorf(...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5db9d19

if !listGVRHasIndexes {
return nil, fmt.Errorf("List on GroupVersionResource %v specifies field selector, but no "+
"indexes for that GroupResourceVersion are defined", gvr)
}

indexExtractor, found := indexes[fieldKey]
if !found {
return nil, fmt.Errorf("no index with name %s was registered", fieldKey)
}

filteredList := make([]runtime.Object, 0, len(list))
for _, obj := range list {
if c.objMatchesFieldSelector(obj, indexExtractor, fieldVal) {
filteredList = append(filteredList, obj)
}
}
return nil
return filteredList, nil
}

func (c *fakeClient) objMatchesFieldSelector(o runtime.Object, extractIndex client.IndexerFunc, val string) bool {
obj, isClientObject := o.(client.Object)
if !isClientObject {
panic(fmt.Errorf("expected object %v to be of type client.Object, but it's not", o))
}

for _, extractedVal := range extractIndex(obj) {
if extractedVal == val {
return true
}
}

return false
}

func (c *fakeClient) Scheme() *runtime.Scheme {
Expand Down
Loading