From 58ae7e565897723e7414e0a78b70e60aec30f5ae Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Tue, 7 Aug 2018 16:50:48 -0700 Subject: [PATCH 1/3] Convert client.List to use functional options Replaces: List(ctx, ListOptions, list) with: List(ctx, list, ...option) This matches the upcoming functional options signature of client.Delete. To use the previous builder options, use the UseListOptions functional option: lo := &client.ListOptions{} client.List(ctx, list, client.UseListOptions( lo.InNamespace(ns).MatchingLabels(labels) )) --- pkg/client/client.go | 13 +-- pkg/client/client_test.go | 240 ++++++++++++++++++++++++++++---------- pkg/client/interfaces.go | 64 +++++++--- 3 files changed, 234 insertions(+), 83 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 195d180dda..a1e1478eae 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -140,20 +140,19 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj runtime.Object) err } // List implements client.Client -func (c *client) List(ctx context.Context, opts *ListOptions, obj runtime.Object) error { +func (c *client) List(ctx context.Context, obj runtime.Object, opts ...ListOptionFunc) error { r, err := c.cache.getResource(obj) if err != nil { return err } - namespace := "" - if opts != nil { - namespace = opts.Namespace - } + + listOpts := ListOptions{} + listOpts.ApplyOptions(opts) return r.Get(). - NamespaceIfScoped(namespace, r.isNamespaced()). + NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()). Resource(r.resource()). Body(obj). - VersionedParams(opts.AsListOptions(), c.paramCodec). + VersionedParams(listOpts.AsListOptions(), c.paramCodec). Do(). Into(obj) } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 072322c8ed..4a91153f39 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -763,7 +763,7 @@ var _ = Describe("Client", func() { By("listing all objects of that type in the cluster") deps := &appsv1.DeploymentList{} - Expect(cl.List(context.Background(), nil, deps)).NotTo(HaveOccurred()) + Expect(cl.List(context.Background(), deps)).NotTo(HaveOccurred()) Expect(deps.Items).NotTo(BeEmpty()) hasDep := false @@ -784,7 +784,7 @@ var _ = Describe("Client", func() { By("listing all Deployments in the cluster") deps := &appsv1.DeploymentList{} - Expect(cl.List(context.Background(), nil, deps)).NotTo(HaveOccurred()) + Expect(cl.List(context.Background(), deps)).NotTo(HaveOccurred()) By("validating no Deployments are returned") Expect(deps.Items).To(BeEmpty()) @@ -793,59 +793,69 @@ var _ = Describe("Client", func() { }, serverSideTimeoutSeconds) // TODO(seans): get label selector test working - // It("should filter results by label selector", func(done Done) { - // By("creating a Deployment with the app=frontend label") - // depFrontend := &appsv1.Deployment{ - // ObjectMeta: metav1.ObjectMeta{Name: "deployment-frontend", Namespace: ns}, - // Spec: appsv1.DeploymentSpec{ - // Selector: &metav1.LabelSelector{ - // MatchLabels: map[string]string{"app": "frontend"}, - // }, - // Template: corev1.PodTemplateSpec{ - // ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "frontend"}}, - // Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, - // }, - // }, - // } - // depFrontend, err := clientset.AppsV1().Deployments(ns).Create(depFrontend) - // Expect(err).NotTo(HaveOccurred()) - - // By("creating a Deployment with the app=backend label") - // depBackend := &appsv1.Deployment{ - // ObjectMeta: metav1.ObjectMeta{Name: "deployment-backend", Namespace: ns}, - // Spec: appsv1.DeploymentSpec{ - // Selector: &metav1.LabelSelector{ - // MatchLabels: map[string]string{"app": "backend"}, - // }, - // Template: corev1.PodTemplateSpec{ - // ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "backend"}}, - // Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, - // }, - // }, - // } - // depBackend, err = clientset.AppsV1().Deployments(ns).Create(depBackend) - // Expect(err).NotTo(HaveOccurred()) - - // cl, err := client.New(cfg, client.Options{}) - // Expect(err).NotTo(HaveOccurred()) - - // By("listing all Deployments with label app=backend") - // deps := &appsv1.DeploymentList{} - // labels := map[string]string{"app": "backend"} - // lo := client.InNamespace(ns).MatchingLabels(labels) - // Expect(cl.List(context.Background(), lo, deps)).NotTo(HaveOccurred()) - - // By("only the Deployment with the backend label is returned") - // Expect(deps.Items).NotTo(BeEmpty()) - // Expect(1).To(Equal(len(deps.Items))) - // actual := deps.Items[0] - // Expect(actual.Name).To(Equal("deployment-backend")) - - // deleteDeployment(depFrontend, ns) - // deleteDeployment(depBackend, ns) - - // close(done) - // }, serverSideTimeoutSeconds) + It("should filter results by label selector", func(done Done) { + By("creating a Deployment with the app=frontend label") + depFrontend := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-frontend", + Namespace: ns, + Labels: map[string]string{"app": "frontend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "frontend"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "frontend"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, + }, + }, + } + depFrontend, err := clientset.AppsV1().Deployments(ns).Create(depFrontend) + Expect(err).NotTo(HaveOccurred()) + + By("creating a Deployment with the app=backend label") + depBackend := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-backend", + Namespace: ns, + Labels: map[string]string{"app": "backend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "backend"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "backend"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, + }, + }, + } + depBackend, err = clientset.AppsV1().Deployments(ns).Create(depBackend) + Expect(err).NotTo(HaveOccurred()) + + cl, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + By("listing all Deployments with label app=backend") + deps := &appsv1.DeploymentList{} + labels := map[string]string{"app": "backend"} + err = cl.List(context.Background(), deps, + client.MatchingLabels(labels), + ) + Expect(err).NotTo(HaveOccurred()) + + By("only the Deployment with the backend label is returned") + Expect(deps.Items).NotTo(BeEmpty()) + Expect(1).To(Equal(len(deps.Items))) + actual := deps.Items[0] + Expect(actual.Name).To(Equal("deployment-backend")) + + deleteDeployment(depFrontend, ns) + deleteDeployment(depBackend, ns) + + close(done) + }, serverSideTimeoutSeconds) It("should filter results by namespace selector", func(done Done) { By("creating a Deployment in test-namespace-1") @@ -891,8 +901,8 @@ var _ = Describe("Client", func() { By("listing all Deployments in test-namespace-1") deps := &appsv1.DeploymentList{} - lo := client.InNamespace("test-namespace-1") - Expect(cl.List(context.Background(), lo, deps)).NotTo(HaveOccurred()) + err = cl.List(context.Background(), deps, client.InNamespace("test-namespace-1")) + Expect(err).NotTo(HaveOccurred()) By("only the Deployment in test-namespace-1 is returned") Expect(deps.Items).NotTo(BeEmpty()) @@ -946,8 +956,8 @@ var _ = Describe("Client", func() { By("listing all Deployments with field metadata.name=deployment-backend") deps := &appsv1.DeploymentList{} - lo := client.MatchingField("metadata.name", "deployment-backend") - Expect(cl.List(context.Background(), lo, deps)).NotTo(HaveOccurred()) + err = cl.List(context.Background(), deps, client.MatchingField("metadata.name", "deployment-backend")) + Expect(err).NotTo(HaveOccurred()) By("only the Deployment with the backend field is returned") Expect(deps.Items).NotTo(BeEmpty()) @@ -961,6 +971,101 @@ var _ = Describe("Client", func() { close(done) }, serverSideTimeoutSeconds) + It("should filter results by namespace selector and label selector", func(done Done) { + By("creating a Deployment in test-namespace-3 with the app=frontend label") + tns3 := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace-3"}} + _, err := clientset.CoreV1().Namespaces().Create(tns3) + Expect(err).NotTo(HaveOccurred()) + depFrontend3 := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-frontend", + Namespace: "test-namespace-3", + Labels: map[string]string{"app": "frontend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "frontend"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "frontend"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, + }, + }, + } + depFrontend3, err = clientset.AppsV1().Deployments("test-namespace-3").Create(depFrontend3) + Expect(err).NotTo(HaveOccurred()) + + By("creating a Deployment in test-namespace-3 with the app=backend label") + depBackend3 := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-backend", + Namespace: "test-namespace-3", + Labels: map[string]string{"app": "backend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "backend"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "backend"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, + }, + }, + } + depBackend3, err = clientset.AppsV1().Deployments("test-namespace-3").Create(depBackend3) + Expect(err).NotTo(HaveOccurred()) + + By("creating a Deployment in test-namespace-4 with the app=frontend label") + tns4 := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace-4"}} + _, err = clientset.CoreV1().Namespaces().Create(tns4) + Expect(err).NotTo(HaveOccurred()) + depFrontend4 := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-frontend", + Namespace: "test-namespace-4", + Labels: map[string]string{"app": "frontend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "frontend"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "frontend"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "nginx", Image: "nginx"}}}, + }, + }, + } + depFrontend4, err = clientset.AppsV1().Deployments("test-namespace-4").Create(depFrontend4) + Expect(err).NotTo(HaveOccurred()) + + cl, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + By("listing all Deployments in test-namespace-3 with label app=frontend") + deps := &appsv1.DeploymentList{} + labels := map[string]string{"app": "frontend"} + err = cl.List(context.Background(), deps, + client.InNamespace("test-namespace-3"), + client.MatchingLabels(labels), + ) + Expect(err).NotTo(HaveOccurred()) + + By("only the Deployment in test-namespace-3 with label app=frontend is returned") + Expect(deps.Items).NotTo(BeEmpty()) + Expect(1).To(Equal(len(deps.Items))) + actual := deps.Items[0] + Expect(actual.Name).To(Equal("deployment-frontend")) + Expect(actual.Namespace).To(Equal("test-namespace-3")) + + deleteDeployment(depFrontend3, "test-namespace-3") + deleteDeployment(depBackend3, "test-namespace-3") + deleteDeployment(depFrontend4, "test-namespace-4") + deleteNamespace(tns3) + deleteNamespace(tns4) + + close(done) + }, serverSideTimeoutSeconds) + PIt("should fail if it cannot get a client", func() { }) @@ -1074,19 +1179,30 @@ var _ = Describe("Client", func() { It("should be created from MatchingLabels", func() { labels := map[string]string{"foo": "bar"} - lo := client.MatchingLabels(labels) + lo := &client.ListOptions{} + client.MatchingLabels(labels)(lo) Expect(lo).NotTo(BeNil()) Expect(lo.LabelSelector.String()).To(Equal("foo=bar")) }) It("should be created from MatchingField", func() { - lo := client.MatchingField("field1", "bar") + lo := &client.ListOptions{} + client.MatchingField("field1", "bar")(lo) Expect(lo).NotTo(BeNil()) Expect(lo.FieldSelector.String()).To(Equal("field1=bar")) }) It("should be created from InNamespace", func() { - lo := client.InNamespace("test") + lo := &client.ListOptions{} + client.InNamespace("test")(lo) + Expect(lo).NotTo(BeNil()) + Expect(lo.Namespace).To(Equal("test")) + }) + + It("should allow pre-built ListOptions", func() { + lo := &client.ListOptions{} + newLo := &client.ListOptions{} + client.UseListOptions(newLo.InNamespace("test"))(lo) Expect(lo).NotTo(BeNil()) Expect(lo.Namespace).To(Equal("test")) }) diff --git a/pkg/client/interfaces.go b/pkg/client/interfaces.go index 36d0fce620..968a5e6ccf 100644 --- a/pkg/client/interfaces.go +++ b/pkg/client/interfaces.go @@ -51,7 +51,7 @@ type Reader interface { // List retrieves list of objects for a given namespace and list options. On a // successful call, Items field in the list will be populated with the // result returned from the server. - List(ctx context.Context, opts *ListOptions, list runtime.Object) error + List(ctx context.Context, list runtime.Object, opts ...ListOptionFunc) error } // Writer knows how to create, delete, and update Kubernetes objects. @@ -185,7 +185,7 @@ func PropagationPolicy(p metav1.DeletionPropagation) DeleteOptionFunc { } } -// ListOptions contains options for limitting or filtering results. +// ListOptions contains options for limiting or filtering results. // It's generally a subset of metav1.ListOptions, with support for // pre-parsed selectors (since generally, selectors will be executed // against the cache). @@ -248,6 +248,20 @@ func (o *ListOptions) AsListOptions() *metav1.ListOptions { return o.Raw } +// ApplyOptions executes the given ListOptionFuncs and returns the mutated +// ListOptions. +func (o *ListOptions) ApplyOptions(optFuncs []ListOptionFunc) *ListOptions { + for _, optFunc := range optFuncs { + optFunc(o) + } + return o +} + +// ListOptionFunc is a function that mutates a ListOptions struct. It implements +// the functional options pattern. See +// https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md. +type ListOptionFunc func(*ListOptions) + // MatchingLabels is a convenience function that sets the label selector // to match the given labels, and then returns the options. // It mutates the list options. @@ -273,20 +287,42 @@ func (o *ListOptions) InNamespace(ns string) *ListOptions { return o } -// MatchingLabels is a convenience function that constructs list options -// to match the given labels. -func MatchingLabels(lbls map[string]string) *ListOptions { - return (&ListOptions{}).MatchingLabels(lbls) +// MatchingLabels is a functional option that sets the LabelSelector field of +// a ListOptions struct. +func MatchingLabels(lbls map[string]string) ListOptionFunc { + sel := labels.SelectorFromSet(lbls) + return func(opts *ListOptions) { + opts.LabelSelector = sel + } +} + +// MatchingField is a functional option that sets the FieldSelector field of +// a ListOptions struct. +func MatchingField(name, val string) ListOptionFunc { + sel := fields.SelectorFromSet(fields.Set{name: val}) + return func(opts *ListOptions) { + opts.FieldSelector = sel + } } -// MatchingField is a convenience function that constructs list options -// to match the given field. -func MatchingField(name, val string) *ListOptions { - return (&ListOptions{}).MatchingField(name, val) +// InNamespace is a functional option that sets the Namespace field of +// a ListOptions struct. +func InNamespace(ns string) ListOptionFunc { + return func(opts *ListOptions) { + opts.Namespace = ns + } } -// InNamespace is a convenience function that constructs list -// options to list in the given namespace. -func InNamespace(ns string) *ListOptions { - return (&ListOptions{}).InNamespace(ns) +// UseListOptions is a functional option that replaces the fields of a +// ListOptions struct with those of a different ListOptions struct. +// +// Example: +// cl.List(ctx, list, client.UseListOptions(lo.InNamespace(ns).MatchingLabels(labels))) +func UseListOptions(newOpts *ListOptions) ListOptionFunc { + return func(opts *ListOptions) { + opts.LabelSelector = newOpts.LabelSelector + opts.FieldSelector = newOpts.FieldSelector + opts.Namespace = newOpts.Namespace + opts.Raw = newOpts.Raw + } } From 8ca92a0ae98c0c65624843324acb974733572e2f Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Thu, 6 Sep 2018 14:54:37 -0700 Subject: [PATCH 2/3] Update List implementations to use new signature The fake client now uses similar GVK detection code as the other implementations and no longer relies on the Raw object having a TypeMeta. --- pkg/cache/cache_test.go | 21 ++++++++++-------- pkg/cache/informer_cache.go | 4 ++-- pkg/cache/informertest/fake_cache.go | 2 +- pkg/cache/internal/cache_reader.go | 19 ++++++++++------- pkg/client/fake/client.go | 23 ++++++++++++++++---- pkg/client/fake/client_test.go | 32 +++++----------------------- 6 files changed, 50 insertions(+), 51 deletions(-) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 82441777d8..1a2d71d12f 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -113,7 +113,7 @@ var _ = Describe("Informer Cache", func() { It("should be able to list objects that haven't been watched previously", func() { By("listing all services in the cluster") listObj := &kcorev1.ServiceList{} - Expect(informerCache.List(context.Background(), nil, listObj)).To(Succeed()) + Expect(informerCache.List(context.Background(), listObj)).To(Succeed()) By("verifying that the returned list contains the Kubernetes service") // NB: kubernetes default service is automatically created in testenv. @@ -143,8 +143,10 @@ var _ = Describe("Informer Cache", func() { By("listing pods with a particular label") // NB: each pod has a "test-label": out := kcorev1.PodList{} - Expect(informerCache.List(context.Background(), client.InNamespace(testNamespaceTwo). - MatchingLabels(map[string]string{"test-label": "test-pod-2"}), &out)).To(Succeed()) + Expect(informerCache.List(context.Background(), &out, + client.InNamespace(testNamespaceTwo), + client.MatchingLabels(map[string]string{"test-label": "test-pod-2"}), + )).To(Succeed()) By("verifying the returned pods have the correct label") Expect(out.Items).NotTo(BeEmpty()) @@ -161,8 +163,9 @@ var _ = Describe("Informer Cache", func() { // NB: each pod has a "test-label": out := kcorev1.PodList{} labels := map[string]string{"test-label": "test-pod-2"} - Expect(informerCache.List(context.Background(), - client.MatchingLabels(labels), &out)).To(Succeed()) + Expect(informerCache.List(context.Background(), &out, + client.MatchingLabels(labels), + )).To(Succeed()) By("verifying multiple pods with the same label in different namespaces are returned") Expect(out.Items).NotTo(BeEmpty()) @@ -177,9 +180,9 @@ var _ = Describe("Informer Cache", func() { It("should be able to list objects by namespace", func() { By("listing pods in test-namespace-1") listObj := &kcorev1.PodList{} - Expect(informerCache.List(context.Background(), + Expect(informerCache.List(context.Background(), listObj, client.InNamespace(testNamespaceOne), - listObj)).To(Succeed()) + )).To(Succeed()) By("verifying that the returned pods are in test-namespace-1") Expect(listObj.Items).NotTo(BeEmpty()) @@ -317,9 +320,9 @@ var _ = Describe("Informer Cache", func() { By("listing Pods with restartPolicyOnFailure") listObj := &kcorev1.PodList{} - Expect(informer.List(context.Background(), + Expect(informer.List(context.Background(), listObj, client.MatchingField("spec.restartPolicy", "OnFailure"), - listObj)).To(Succeed()) + )).To(Succeed()) By("verifying that the returned pods have correct restart policy") Expect(listObj.Items).NotTo(BeEmpty()) diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index 03298baeeb..4e132fa3af 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -57,7 +57,7 @@ func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out runt } // List implements Reader -func (ip *informerCache) List(ctx context.Context, opts *client.ListOptions, out runtime.Object) error { +func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...client.ListOptionFunc) error { itemsPtr, err := apimeta.GetItemsPtr(out) if err != nil { return nil @@ -87,7 +87,7 @@ func (ip *informerCache) List(ctx context.Context, opts *client.ListOptions, out return err } - return cache.Reader.List(ctx, opts, out) + return cache.Reader.List(ctx, out, opts...) } // GetInformerForKind returns the informer for the GroupVersionKind diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index 87e6aeedcb..2f311cf729 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -136,6 +136,6 @@ func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj runti } // List implements Cache -func (c *FakeInformers) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { +func (c *FakeInformers) List(ctx context.Context, list runtime.Object, opts ...client.ListOptionFunc) error { return nil } diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go index 199d0bf835..281a0fe826 100644 --- a/pkg/cache/internal/cache_reader.go +++ b/pkg/cache/internal/cache_reader.go @@ -86,23 +86,26 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out runtime.O } // List lists items out of the indexer and writes them to out -func (c *CacheReader) List(ctx context.Context, opts *client.ListOptions, out runtime.Object) error { +func (c *CacheReader) List(ctx context.Context, out runtime.Object, opts ...client.ListOptionFunc) error { var objs []interface{} var err error - if opts != nil && opts.FieldSelector != nil { + listOpts := client.ListOptions{} + listOpts.ApplyOptions(opts) + + if listOpts.FieldSelector != nil { // TODO(directxman12): support more complicated field selectors by // combining multiple indicies, GetIndexers, etc - field, val, requiresExact := requiresExactMatch(opts.FieldSelector) + field, val, requiresExact := 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(opts.Namespace, val)) - } else if opts != nil && opts.Namespace != "" { - objs, err = c.indexer.ByIndex(cache.NamespaceIndex, opts.Namespace) + objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(listOpts.Namespace, val)) + } else if listOpts.Namespace != "" { + objs, err = c.indexer.ByIndex(cache.NamespaceIndex, listOpts.Namespace) } else { objs = c.indexer.List() } @@ -110,8 +113,8 @@ func (c *CacheReader) List(ctx context.Context, opts *client.ListOptions, out ru return err } var labelSel labels.Selector - if opts != nil && opts.LabelSelector != nil { - labelSel = opts.LabelSelector + if listOpts.LabelSelector != nil { + labelSel = listOpts.LabelSelector } outItems, err := c.getListItems(objs, labelSel) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 8597cbde6c..657d031dca 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -19,7 +19,9 @@ package fake import ( "context" "encoding/json" + "fmt" "os" + "strings" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" @@ -77,10 +79,23 @@ func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj runtime. return err } -func (c *fakeClient) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { - gvk := opts.Raw.TypeMeta.GroupVersionKind() +func (c *fakeClient) List(ctx context.Context, obj runtime.Object, opts ...client.ListOptionFunc) error { + gvk, err := apiutil.GVKForObject(obj, scheme.Scheme) + if err != nil { + return err + } + + if !strings.HasSuffix(gvk.Kind, "List") { + return fmt.Errorf("non-list type %T (kind %q) passed as output", obj, gvk) + } + // we need the non-list GVK, so chop off the "List" from the end of the kind + gvk.Kind = gvk.Kind[:len(gvk.Kind)-4] + + listOpts := client.ListOptions{} + listOpts.ApplyOptions(opts) + gvr, _ := meta.UnsafeGuessKindToResource(gvk) - o, err := c.tracker.List(gvr, gvk, opts.Namespace) + o, err := c.tracker.List(gvr, gvk, listOpts.Namespace) if err != nil { return err } @@ -89,7 +104,7 @@ func (c *fakeClient) List(ctx context.Context, opts *client.ListOptions, list ru return err } decoder := scheme.Codecs.UniversalDecoder() - _, _, err = decoder.Decode(j, nil, list) + _, _, err = decoder.Decode(j, nil, obj) return err } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 635c8c1058..872686c7bf 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -17,15 +17,12 @@ limitations under the License. package fake import ( - "encoding/json" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -69,22 +66,11 @@ var _ = Describe("Fake client", func() { It("should be able to List", func() { By("Listing all deployments in a namespace") - list := &metav1.List{} - err := cl.List(nil, &client.ListOptions{ - Namespace: "ns1", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "apps/v1", - Kind: "Deployment", - }, - }, - }, list) + list := &appsv1.DeploymentList{} + err := cl.List(nil, list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(1)) - j, err := json.Marshal(dep) - Expect(err).To(BeNil()) - expectedDep := runtime.RawExtension{Raw: j} - Expect(list.Items).To(ConsistOf(expectedDep)) + Expect(list.Items).To(ConsistOf(*dep)) }) It("should be able to Create", func() { @@ -140,16 +126,8 @@ var _ = Describe("Fake client", func() { Expect(err).To(BeNil()) By("Listing all deployments in the namespace") - list := &metav1.List{} - err = cl.List(nil, &client.ListOptions{ - Namespace: "ns1", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "apps/v1", - Kind: "Deployment", - }, - }, - }, list) + list := &appsv1.DeploymentList{} + err = cl.List(nil, list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(0)) }) From bf0795d40d9a624b0c971c664b03a3770739fafc Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Thu, 6 Sep 2018 14:56:26 -0700 Subject: [PATCH 3/3] Update tests to use new List signature Now that the fake client no longer needs Raw, these tests can use typed instead of untyped List objects. --- pkg/builder/example_test.go | 2 +- .../internal/cert/writer/secret_test.go | 89 +++---------------- 2 files changed, 15 insertions(+), 76 deletions(-) diff --git a/pkg/builder/example_test.go b/pkg/builder/example_test.go index 8339b56087..21f8500f6b 100644 --- a/pkg/builder/example_test.go +++ b/pkg/builder/example_test.go @@ -77,7 +77,7 @@ func (a *ReplicaSetReconciler) Reconcile(req reconcile.Request) (reconcile.Resul // List the Pods matching the PodTemplate Labels pods := &corev1.PodList{} - err = a.List(context.TODO(), client.InNamespace(req.Namespace).MatchingLabels(rs.Spec.Template.Labels), pods) + err = a.List(context.TODO(), pods, client.InNamespace(req.Namespace), client.MatchingLabels(rs.Spec.Template.Labels)) if err != nil { return reconcile.Result{}, err } diff --git a/pkg/webhook/internal/cert/writer/secret_test.go b/pkg/webhook/internal/cert/writer/secret_test.go index 0090f9add1..c784ec725d 100644 --- a/pkg/webhook/internal/cert/writer/secret_test.go +++ b/pkg/webhook/internal/cert/writer/secret_test.go @@ -17,14 +17,11 @@ limitations under the License. package writer import ( - "encoding/json" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -38,7 +35,6 @@ var _ = Describe("secretCertWriter", func() { var certWriter CertWriter var sCertWriter *secretCertWriter var secret *corev1.Secret - var expectedSecret runtime.RawExtension BeforeEach(func(done Done) { var err error @@ -105,43 +101,21 @@ var _ = Describe("secretCertWriter", func() { It("should default it and return no error", func() { _, _, err := certWriter.EnsureCert(dnsName, false) Expect(err).NotTo(HaveOccurred()) - list := &corev1.List{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Secret", - }, - }, - }, list) + list := &corev1.SecretList{} + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(HaveLen(1)) }) }) Context("no existing secret", func() { - BeforeEach(func(done Done) { - j, _ := json.Marshal(secret) - expectedSecret = runtime.RawExtension{Raw: j} - close(done) - }) - It("should create new secrets with certs", func() { _, changed, err := certWriter.EnsureCert(dnsName, false) Expect(err).NotTo(HaveOccurred()) - list := &corev1.List{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Secret", - }, - }, - }, list) + list := &corev1.SecretList{} + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) - Expect(list.Items).To(ConsistOf(expectedSecret)) + Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) Expect(changed).To(BeTrue()) }) @@ -151,12 +125,6 @@ var _ = Describe("secretCertWriter", func() { var oldSecret *corev1.Secret Context("cert is invalid", func() { - BeforeEach(func(done Done) { - j, _ := json.Marshal(secret) - expectedSecret = runtime.RawExtension{Raw: j} - close(done) - }) - Describe("cert in secret is incomplete", func() { BeforeEach(func(done Done) { oldSecret = secret.DeepCopy() @@ -168,18 +136,10 @@ var _ = Describe("secretCertWriter", func() { It("should replace with new certs", func() { _, changed, err := certWriter.EnsureCert(dnsName, false) Expect(err).NotTo(HaveOccurred()) - list := &corev1.List{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Secret", - }, - }, - }, list) + list := &corev1.SecretList{} + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) - Expect(list.Items).To(ConsistOf(expectedSecret)) + Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) Expect(changed).To(BeTrue()) }) @@ -200,18 +160,10 @@ var _ = Describe("secretCertWriter", func() { It("should replace with new certs", func() { _, changed, err := certWriter.EnsureCert(dnsName, false) Expect(err).NotTo(HaveOccurred()) - list := &corev1.List{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Secret", - }, - }, - }, list) + list := &corev1.SecretList{} + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) - Expect(list.Items).To(ConsistOf(expectedSecret)) + Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) Expect(changed).To(BeTrue()) }) @@ -225,8 +177,6 @@ var _ = Describe("secretCertWriter", func() { ServerKeyName: []byte(certs2.Key), ServerCertName: []byte(certs2.Cert), } - j, _ := json.Marshal(oldSecret) - expectedSecret = runtime.RawExtension{Raw: j} sCertWriter.Client = fake.NewFakeClient(oldSecret) close(done) }) @@ -239,28 +189,17 @@ var _ = Describe("secretCertWriter", func() { ServerKeyName: []byte(certs2.Key), ServerCertName: []byte(certs2.Cert), } - j, _ := json.Marshal(oldSecret) - expectedSecret = runtime.RawExtension{Raw: j} - sCertWriter.Client = fake.NewFakeClient(oldSecret) close(done) }) It("should keep the secret", func() { _, changed, err := certWriter.EnsureCert(dnsName, false) Expect(err).NotTo(HaveOccurred()) - list := &corev1.List{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - Raw: &metav1.ListOptions{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Secret", - }, - }, - }, list) + list := &corev1.SecretList{} + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(HaveLen(1)) - Expect(list.Items[0]).To(Equal(expectedSecret)) + Expect(list.Items).To(ConsistOf(*oldSecret)) Expect(changed).To(BeFalse()) }) })