Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@matteoolivi
matteoolivi committed Oct 24, 2022
1 parent c8aad1d commit 5db9d19
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 51 deletions.
71 changes: 40 additions & 31 deletions pkg/client/fake/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ type fakeClient struct {
scheme *runtime.Scheme
restMapper meta.RESTMapper

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

schemeWriteLock sync.Mutex
}
Expand Down Expand Up @@ -102,9 +102,9 @@ type ClientBuilder struct {
initRuntimeObjects []runtime.Object
objectTracker testing.ObjectTracker

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

// WithScheme sets this builder's internal scheme.
Expand Down Expand Up @@ -147,27 +147,40 @@ 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 {
// WithIndex can be optionally used to register an index with name `field` and indexer `extractValue`
// for API objects of the same GroupVersionKind (GVK) as `obj` in the fake client.
// It can be invoked multiple times, both with objects of the same GVK or different ones.
// Invoking WithIndex twice with the same `field` and GVK (via `obj`) arguments will panic.
// WithIndex retrieves the GVK of `obj` using the scheme registered via WithScheme if
// WithScheme was previously invoked, the default scheme otherwise.
func (f *ClientBuilder) WithIndex(obj runtime.Object, field string, extractValue client.IndexerFunc) *ClientBuilder {
objScheme := f.scheme
if objScheme == nil {
objScheme = scheme.Scheme
}

gvk, err := apiutil.GVKForObject(obj, objScheme)
if err != nil {
panic(err)
}

// 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)
f.indexes = make(map[schema.GroupVersionKind]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 this is the first index being registered for the GroupVersionKind of `obj`, we initialize
// the map storing the indexes for that GroupVersionKind.
if f.indexes[gvk] == nil {
f.indexes[gvk] = 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))
if _, fieldAlreadyIndexed := f.indexes[gvk][field]; fieldAlreadyIndexed {
panic(fmt.Errorf("indexer conflict: field %s for GroupVersionKind %v is already indexed",
field, gvk))
}

f.indexes[gvr][name] = indexer
f.indexes[gvk][field] = extractValue

return f
}
Expand Down Expand Up @@ -469,15 +482,15 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
return err
}

filteredList, err := c.filterList(objs, gvr, listOpts.LabelSelector, listOpts.FieldSelector)
filteredList, err := c.filterList(objs, gvk, 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) {
func (c *fakeClient) filterList(list []runtime.Object, gvk schema.GroupVersionKind, ls labels.Selector, fs fields.Selector) ([]runtime.Object, error) {
// Filter the objects with the label selector
filteredList := list
if ls != nil {
Expand All @@ -490,7 +503,7 @@ func (c *fakeClient) filterList(list []runtime.Object, gvr schema.GroupVersionRe

// Filter the result of the previous pass with the field selector
if fs != nil {
objsFilteredByField, err := c.filterWithFields(filteredList, gvr, fs)
objsFilteredByField, err := c.filterWithFields(filteredList, gvk, fs)
if err != nil {
return nil, err
}
Expand All @@ -500,7 +513,7 @@ func (c *fakeClient) filterList(list []runtime.Object, gvr schema.GroupVersionRe
return filteredList, nil
}

func (c *fakeClient) filterWithFields(list []runtime.Object, gvr schema.GroupVersionResource, fs fields.Selector) ([]runtime.Object, error) {
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)
Expand All @@ -510,18 +523,14 @@ func (c *fakeClient) filterWithFields(list []runtime.Object, gvr 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 GroupVersionResource of the objects in the list.
indexes, listGVRHasIndexes := c.indexes[gvr]
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)
// 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)
}

indexExtractor := indexes[fieldKey]
filteredList := make([]runtime.Object, 0, len(list))
for _, obj := range list {
if c.objMatchesFieldSelector(obj, indexExtractor, fieldVal) {
Expand Down
32 changes: 12 additions & 20 deletions pkg/client/fake/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/kubernetes/fake"

appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -100,7 +99,7 @@ var _ = Describe("Fake client", func() {
}
})

AssertClientWOIndexBehavior := func() {
AssertClientWithoutIndexBehavior := func() {
It("should be able to Get", func() {
By("Getting a deployment")
namespacedName := types.NamespacedName{
Expand Down Expand Up @@ -981,7 +980,7 @@ var _ = Describe("Fake client", func() {
WithObjects(dep, dep2, cm).
Build()
})
AssertClientWOIndexBehavior()
AssertClientWithoutIndexBehavior()
})

Context("with given scheme", func() {
Expand All @@ -996,7 +995,7 @@ var _ = Describe("Fake client", func() {
WithLists(&appsv1.DeploymentList{Items: []appsv1.Deployment{*dep, *dep2}}).
Build()
})
AssertClientWOIndexBehavior()
AssertClientWithoutIndexBehavior()
})

Context("with Indexes", func() {
Expand All @@ -1022,23 +1021,18 @@ var _ = Describe("Fake client", func() {
return []string{string(dep.Spec.Strategy.Type)}
}

var (
depGVR schema.GroupVersionResource
cb *ClientBuilder
)

var cb *ClientBuilder
BeforeEach(func() {
cb = NewClientBuilder().
WithObjects(dep, dep2, cm).
WithIndex(depGVR, "spec.replicas", depReplicasIndexer)
depGVR = appsv1.SchemeGroupVersion.WithResource("deployments")
WithIndex(&appsv1.Deployment{}, "spec.replicas", depReplicasIndexer)
})

Context("client has just one Index", func() {
BeforeEach(func() { cl = cb.Build() })

Context("behavior that doesn't use an Index", func() {
AssertClientWOIndexBehavior()
AssertClientWithoutIndexBehavior()
})

Context("filtered List using field selector", func() {
Expand Down Expand Up @@ -1120,11 +1114,11 @@ var _ = Describe("Fake client", func() {

Context("client has two Indexes", func() {
BeforeEach(func() {
cl = cb.WithIndex(depGVR, "spec.strategy.type", depStrategyTypeIndexer).Build()
cl = cb.WithIndex(&appsv1.Deployment{}, "spec.strategy.type", depStrategyTypeIndexer).Build()
})

Context("behavior that doesn't use an Index", func() {
AssertClientWOIndexBehavior()
AssertClientWithoutIndexBehavior()
})

Context("filtered List using field selector", func() {
Expand Down Expand Up @@ -1229,16 +1223,14 @@ var _ = Describe("Fake client", func() {
})

var _ = Describe("Fake client builder", func() {
It("panics when an index with the same name and GroupVersionResource is registered twice", func() {
// We need any realistic GroupVersionResource, the choice of deployments is arbitrary.
gvr := appsv1.SchemeGroupVersion.WithResource("deployments")

cb := NewClientBuilder().WithIndex(gvr,
It("panics when an index with the same name and GroupVersionKind is registered twice", func() {
// We need any realistic GroupVersionKind, the choice of apps/v1 Deployment is arbitrary.
cb := NewClientBuilder().WithIndex(&appsv1.Deployment{},
"test-name",
func(client.Object) []string { return nil })

Expect(func() {
cb.WithIndex(gvr,
cb.WithIndex(&appsv1.Deployment{},
"test-name",
func(client.Object) []string { return []string{"foo"} })
}).To(Panic())
Expand Down

0 comments on commit 5db9d19

Please sign in to comment.