Adding two types of client for unstructured and typed

Author: Shawn Hurley

pkg/cache/cache_test.go

@@ -185,8 +185,9 @@ var _ = Describe("Informer Cache", func() {
 				By("listing pods in test-namespace-1")
 				listObj := &kcorev1.PodList{}
 				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())
@@ -231,7 +232,8 @@ var _ = Describe("Informer Cache", func() {
 					Version: "v1",
 					Kind:    "ServiceList",
 				})
-				Expect(informerCache.List(context.Background(), nil, listObj)).To(Succeed())
+				err := informerCache.List(context.Background(), listObj)
+				Expect(err).To(Succeed())
 
 				By("verifying that the returned list contains the Kubernetes service")
 				// NB: kubernetes default service is automatically created in testenv.
@@ -271,8 +273,9 @@ var _ = Describe("Informer Cache", func() {
 					Version: "v1",
 					Kind:    "PodList",
 				})
-				Expect(informerCache.List(context.Background(), client.InNamespace(testNamespaceTwo).
-					MatchingLabels(map[string]string{"test-label": "test-pod-2"}), &out)).To(Succeed())
+				err := informerCache.List(context.Background(), &out, client.InNamespace(testNamespaceTwo),
+					client.MatchingLabels(map[string]string{"test-label": "test-pod-2"}))
+				Expect(err).To(Succeed())
 
 				By("verifying the returned pods have the correct label")
 				Expect(out.Items).NotTo(BeEmpty())
@@ -294,8 +297,9 @@ var _ = Describe("Informer Cache", func() {
 					Kind:    "PodList",
 				})
 				labels := map[string]string{"test-label": "test-pod-2"}
-				Expect(informerCache.List(context.Background(),
-					client.MatchingLabels(labels), &out)).To(Succeed())
+				err := informerCache.List(context.Background(),
+					&out, client.MatchingLabels(labels))
+				Expect(err).To(Succeed())
 
 				By("verifying multiple pods with the same label in different namespaces are returned")
 				Expect(out.Items).NotTo(BeEmpty())
@@ -315,9 +319,8 @@ var _ = Describe("Informer Cache", func() {
 					Version: "v1",
 					Kind:    "PodList",
 				})
-				Expect(informerCache.List(context.Background(),
-					client.InNamespace(testNamespaceOne),
-					listObj)).To(Succeed())
+				err := informerCache.List(context.Background(), listObj, client.InNamespace(testNamespaceOne))
+				Expect(err).To(Succeed())
 
 				By("verifying that the returned pods are in test-namespace-1")
 				Expect(listObj.Items).NotTo(BeEmpty())
@@ -524,7 +527,7 @@ var _ = Describe("Informer Cache", func() {
 				By("verifying the object is received on the channel")
 				Eventually(out).Should(Receive(Equal(pod)))
 				close(done)
-			})
+			}, 3)
 
 			It("should be able to index an object field then retrieve objects by that field", func() {
 				By("creating the cache")
@@ -565,16 +568,15 @@ var _ = Describe("Informer Cache", func() {
 					Version: "v1",
 					Kind:    "PodList",
 				})
-				Expect(informer.List(context.Background(),
-					client.MatchingField("spec.restartPolicy", "OnFailure"),
-					listObj)).To(Succeed())
+				err = informer.List(context.Background(), listObj, client.MatchingField("spec.restartPolicy", "OnFailure"))
+				Expect(err).To(Succeed())
 
 				By("verifying that the returned pods have correct restart policy")
 				Expect(listObj.Items).NotTo(BeEmpty())
 				Expect(listObj.Items).Should(HaveLen(1))
 				actual := listObj.Items[0]
 				Expect(actual.GetName()).To(Equal("test-pod-3"))
-			})
+			}, 3)
 		})
 	})
 })

pkg/cache/informer_cache.go

@@ -58,12 +58,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 {
-	itemsPtr, err := apimeta.GetItemsPtr(out)
-	if err != nil {
-		return nil
-	}
-
+func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...client.ListOptionFunc) error {
 	gvk, err := apiutil.GVKForObject(out, ip.Scheme)
 	if err != nil {
 		return err

pkg/client/client.go

@@ -22,9 +22,10 @@ import (
 	"reflect"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -59,16 +60,26 @@ func New(config *rest.Config, options Options) (Client, error) {
 		}
 	}
 
+	dynamicClient, err := dynamic.NewForConfig(config)
+	if err != nil {
+		return nil, err
+	}
+
 	c := &client{
-		cache: clientCache{
-			config:                    config,
-			scheme:                    options.Scheme,
-			mapper:                    options.Mapper,
-			codecs:                    serializer.NewCodecFactory(options.Scheme),
-			resourceByType:            make(map[reflect.Type]*resourceMeta),
-			unstructuredResourceByGVK: make(map[schema.GroupVersionKind]*resourceMeta),
+		typedClient: typedClient{
+			cache: clientCache{
+				config:         config,
+				scheme:         options.Scheme,
+				mapper:         options.Mapper,
+				codecs:         serializer.NewCodecFactory(options.Scheme),
+				resourceByType: make(map[reflect.Type]*resourceMeta),
+			},
+			paramCodec: runtime.NewParameterCodec(options.Scheme),
+		},
+		unstructuredClient: unstructuredClient{
+			client:     dynamicClient,
+			restMapper: options.Mapper,
 		},
-		paramCodec: runtime.NewParameterCodec(options.Scheme),
 	}
 
 	return c, nil
@@ -79,84 +90,53 @@ var _ Client = &client{}
 // client is a client.Client that reads and writes directly from/to an API server.  It lazily initializes
 // new clients at the time they are used, and caches the client.
 type client struct {
-	cache      clientCache
-	paramCodec runtime.ParameterCodec
+	typedClient        typedClient
+	unstructuredClient unstructuredClient
 }
 
 // Create implements client.Client
 func (c *client) Create(ctx context.Context, obj runtime.Object) error {
-	o, err := c.cache.getObjMeta(obj)
-	if err != nil {
-		return err
+	_, ok := obj.(*unstructured.Unstructured)
+	if ok {
+		return c.unstructuredClient.Create(ctx, obj)
 	}
-	return o.Post().
-		NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
-		Resource(o.resource()).
-		Body(obj).
-		Do().
-		Into(obj)
+	return c.typedClient.Create(ctx, obj)
 }
 
 // Update implements client.Client
 func (c *client) Update(ctx context.Context, obj runtime.Object) error {
-	o, err := c.cache.getObjMeta(obj)
-	if err != nil {
-		return err
+	_, ok := obj.(*unstructured.Unstructured)
+	if ok {
+		return c.unstructuredClient.Update(ctx, obj)
 	}
-	return o.Put().
-		NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
-		Resource(o.resource()).
-		Name(o.GetName()).
-		Body(obj).
-		Do().
-		Into(obj)
+	return c.typedClient.Update(ctx, obj)
 }
 
 // Delete implements client.Client
 func (c *client) Delete(ctx context.Context, obj runtime.Object, opts ...DeleteOptionFunc) error {
-	o, err := c.cache.getObjMeta(obj)
-	if err != nil {
-		return err
+	_, ok := obj.(*unstructured.Unstructured)
+	if ok {
+		return c.unstructuredClient.Delete(ctx, obj, opts...)
 	}
-
-	deleteOpts := DeleteOptions{}
-	return o.Delete().
-		NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
-		Resource(o.resource()).
-		Name(o.GetName()).
-		Body(deleteOpts.ApplyOptions(opts).AsDeleteOptions()).
-		Do().
-		Error()
+	return c.typedClient.Delete(ctx, obj, opts...)
 }
 
 // Get implements client.Client
 func (c *client) Get(ctx context.Context, key ObjectKey, obj runtime.Object) error {
-	r, err := c.cache.getResource(obj)
-	if err != nil {
-		return err
+	_, ok := obj.(*unstructured.Unstructured)
+	if ok {
+		return c.unstructuredClient.Get(ctx, key, obj)
 	}
-	return r.Get().
-		NamespaceIfScoped(key.Namespace, r.isNamespaced()).
-		Resource(r.resource()).
-		Name(key.Name).Do().Into(obj)
+	return c.typedClient.Get(ctx, key, obj)
 }
 
 // List implements client.Client
 func (c *client) List(ctx context.Context, obj runtime.Object, opts ...ListOptionFunc) error {
-	r, err := c.cache.getResource(obj)
-	if err != nil {
-		return err
+	_, ok := obj.(*unstructured.UnstructuredList)
+	if ok {
+		return c.unstructuredClient.List(ctx, obj, opts...)
 	}
-
-	listOpts := ListOptions{}
-	listOpts.ApplyOptions(opts)
-	return r.Get().
-		NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()).
-		Resource(r.resource()).
-		Body(obj).
-		VersionedParams(listOpts.AsListOptions(), c.paramCodec).
-		Do().
-		Into(obj)
+	return c.typedClient.List(ctx, obj, opts...)
 }
 
 // Status implements client.StatusClient
@@ -174,7 +154,7 @@ var _ StatusWriter = &statusWriter{}
 
 // Update implements client.StatusWriter
 func (sw *statusWriter) Update(_ context.Context, obj runtime.Object) error {
-	o, err := sw.client.cache.getObjMeta(obj)
+	o, err := sw.client.typedClient.cache.getObjMeta(obj)
 	if err != nil {
 		return err
 	}

pkg/client/client_cache.go

@@ -23,7 +23,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -45,18 +44,14 @@ type clientCache struct {
 	// codecs are used to create a REST client for a gvk
 	codecs serializer.CodecFactory
 
-	muByType sync.RWMutex
 	// resourceByType caches type metadata
 	resourceByType map[reflect.Type]*resourceMeta
-
-	muByGVK sync.RWMutex
-	// resourceByGVK caches type metadata for unstructured
-	unstructuredResourceByGVK map[schema.GroupVersionKind]*resourceMeta
+	mu             sync.RWMutex
 }
 
 // newResource maps obj to a Kubernetes Resource and constructs a client for that Resource.
 // If the object is a list, the resource represents the item's type instead.
-func (c *clientCache) newResource(obj runtime.Object, isUnstructured bool) (*resourceMeta, error) {
+func (c *clientCache) newResource(obj runtime.Object) (*resourceMeta, error) {
 	gvk, err := apiutil.GVKForObject(obj, c.scheme)
 	if err != nil {
 		return nil, err
@@ -67,12 +62,7 @@ func (c *clientCache) newResource(obj runtime.Object, isUnstructured bool) (*res
 		gvk.Kind = gvk.Kind[:len(gvk.Kind)-4]
 	}
 
-	var client rest.Interface
-	if isUnstructured {
-		client, err = apiutil.RESTUnstructuredClientForGVK(gvk, c.config)
-	} else {
-		client, err = apiutil.RESTClientForGVK(gvk, c.config, c.codecs)
-	}
+	client, err := apiutil.RESTClientForGVK(gvk, c.config, c.codecs)
 	if err != nil {
 		return nil, err
 	}
@@ -83,63 +73,32 @@ func (c *clientCache) newResource(obj runtime.Object, isUnstructured bool) (*res
 	return &resourceMeta{Interface: client, mapping: mapping, gvk: gvk}, nil
 }
 
-func (c *clientCache) getUnstructuredResourceByGVK(obj runtime.Object) (*resourceMeta, error) {
-	// It's better to do creation work twice than to not let multiple
-	// people make requests at once
-	c.muByGVK.RLock()
-	r, known := c.unstructuredResourceByGVK[obj.GetObjectKind().GroupVersionKind()]
-	c.muByGVK.RUnlock()
-
-	if known {
-		return r, nil
-	}
-
-	// Initialize a new Client
-	c.muByGVK.Lock()
-	defer c.muByGVK.Unlock()
-	r, err := c.newResource(obj, true)
-	if err != nil {
-		return nil, err
-	}
-	c.unstructuredResourceByGVK[obj.GetObjectKind().GroupVersionKind()] = r
-	return r, err
-}
-
-func (c *clientCache) getResourceByType(obj runtime.Object) (*resourceMeta, error) {
+// getResource returns the resource meta information for the given type of object.
+// If the object is a list, the resource represents the item's type instead.
+func (c *clientCache) getResource(obj runtime.Object) (*resourceMeta, error) {
 	typ := reflect.TypeOf(obj)
 
 	// It's better to do creation work twice than to not let multiple
 	// people make requests at once
-	c.muByType.RLock()
+	c.mu.RLock()
 	r, known := c.resourceByType[typ]
-	c.muByType.RUnlock()
+	c.mu.RUnlock()
 
 	if known {
 		return r, nil
 	}
 
 	// Initialize a new Client
-	c.muByType.Lock()
-	defer c.muByType.Unlock()
-	r, err := c.newResource(obj, false)
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	r, err := c.newResource(obj)
 	if err != nil {
 		return nil, err
 	}
 	c.resourceByType[typ] = r
 	return r, err
 }
 
-// getResource returns the resource meta information for the given type of object.
-// If the object is a list, the resource represents the item's type instead.
-func (c *clientCache) getResource(obj runtime.Object) (*resourceMeta, error) {
-	_, isUnstructured := obj.(*unstructured.Unstructured)
-	_, isUnstructuredList := obj.(*unstructured.UnstructuredList)
-	if isUnstructured || isUnstructuredList {
-		return c.getUnstructuredResourceByGVK(obj)
-	}
-	return c.getResourceByType(obj)
-}
-
 // getObjMeta returns objMeta containing both type and object metadata and state
 func (c *clientCache) getObjMeta(obj runtime.Object) (*objMeta, error) {
 	r, err := c.getResource(obj)