pkg/client/client_test.go

@@ -22,8 +22,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"k8s.io/apimachinery/pkg/types"
-
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
@@ -33,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -3106,48 +3105,79 @@ var _ = Describe("DelegatingClient", func() {
 			Expect(1).To(Equal(cachedReader.Called))
 		})
 
-		It("should call client reader when unstructured object", func() {
-			cachedReader := &fakeReader{}
-			cl, err := client.New(cfg, client.Options{})
-			Expect(err).NotTo(HaveOccurred())
-			dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
-				CacheReader: cachedReader,
-				Client:      cl,
-			})
-			Expect(err).NotTo(HaveOccurred())
-			dep := &appsv1.Deployment{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "deployment1",
-					Labels: map[string]string{"app": "frontend"},
-				},
-				Spec: appsv1.DeploymentSpec{
-					Selector: &metav1.LabelSelector{
-						MatchLabels: map[string]string{"app": "frontend"},
+		When("getting unstructured objects", func() {
+			var dep *appsv1.Deployment
+
+			BeforeEach(func() {
+				dep = &appsv1.Deployment{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:   "deployment1",
+						Labels: map[string]string{"app": "frontend"},
 					},
-					Template: corev1.PodTemplateSpec{
-						ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "frontend"}},
-						Spec:       corev1.PodSpec{Containers: []corev1.Container{{Name: "x", Image: "x"}}},
+					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: "x", Image: "x"}}},
+						},
 					},
-				},
-			}
-			dep, err = clientset.AppsV1().Deployments("default").Create(context.Background(), dep, metav1.CreateOptions{})
-			Expect(err).NotTo(HaveOccurred())
+				}
+				var err error
+				dep, err = clientset.AppsV1().Deployments("default").Create(context.Background(), dep, metav1.CreateOptions{})
+				Expect(err).NotTo(HaveOccurred())
+			})
+			AfterEach(func() {
+				Expect(clientset.AppsV1().Deployments("default").Delete(
+					context.Background(),
+					dep.Name,
+					metav1.DeleteOptions{},
+				)).To(Succeed())
+			})
+			It("should call client reader when not cached", func() {
+				cachedReader := &fakeReader{}
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
+					CacheReader: cachedReader,
+					Client:      cl,
+				})
+				Expect(err).NotTo(HaveOccurred())
+
+				actual := &unstructured.Unstructured{}
+				actual.SetGroupVersionKind(schema.GroupVersionKind{
+					Group:   "apps",
+					Kind:    "Deployment",
+					Version: "v1",
+				})
+				actual.SetName(dep.Name)
+				key := client.ObjectKey{Namespace: dep.Namespace, Name: dep.Name}
+				Expect(dReader.Get(context.TODO(), key, actual)).To(Succeed())
+				Expect(0).To(Equal(cachedReader.Called))
+			})
+			It("should call cache reader when cached", func() {
+				cachedReader := &fakeReader{}
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
+					CacheReader:       cachedReader,
+					Client:            cl,
+					CacheUnstructured: true,
+				})
+				Expect(err).NotTo(HaveOccurred())
 
-			actual := &unstructured.Unstructured{}
-			actual.SetGroupVersionKind(schema.GroupVersionKind{
-				Group:   "apps",
-				Kind:    "Deployment",
-				Version: "v1",
-			})
-			actual.SetName(dep.Name)
-			key := client.ObjectKey{Namespace: dep.Namespace, Name: dep.Name}
-			Expect(dReader.Get(context.TODO(), key, actual)).To(Succeed())
-			Expect(0).To(Equal(cachedReader.Called))
-			Expect(clientset.AppsV1().Deployments("default").Delete(
-				context.Background(),
-				dep.Name,
-				metav1.DeleteOptions{},
-			)).To(Succeed())
+				actual := &unstructured.Unstructured{}
+				actual.SetGroupVersionKind(schema.GroupVersionKind{
+					Group:   "apps",
+					Kind:    "Deployment",
+					Version: "v1",
+				})
+				actual.SetName(dep.Name)
+				key := client.ObjectKey{Namespace: dep.Namespace, Name: dep.Name}
+				Expect(dReader.Get(context.TODO(), key, actual)).To(Succeed())
+				Expect(1).To(Equal(cachedReader.Called))
+			})
 		})
 	})
 	Describe("List", func() {
@@ -3165,24 +3195,46 @@ var _ = Describe("DelegatingClient", func() {
 			Expect(1).To(Equal(cachedReader.Called))
 		})
 
-		It("should call client reader when unstructured object", func() {
-			cachedReader := &fakeReader{}
-			cl, err := client.New(cfg, client.Options{})
-			Expect(err).NotTo(HaveOccurred())
-			dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
-				CacheReader: cachedReader,
-				Client:      cl,
+		When("listing unstructured objects", func() {
+			It("should call client reader when not cached", func() {
+				cachedReader := &fakeReader{}
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
+					CacheReader: cachedReader,
+					Client:      cl,
+				})
+				Expect(err).NotTo(HaveOccurred())
+
+				actual := &unstructured.UnstructuredList{}
+				actual.SetGroupVersionKind(schema.GroupVersionKind{
+					Group:   "apps",
+					Kind:    "DeploymentList",
+					Version: "v1",
+				})
+				Expect(dReader.List(context.Background(), actual)).To(Succeed())
+				Expect(0).To(Equal(cachedReader.Called))
 			})
-			Expect(err).NotTo(HaveOccurred())
+			It("should call cache reader when cached", func() {
+				cachedReader := &fakeReader{}
+				cl, err := client.New(cfg, client.Options{})
+				Expect(err).NotTo(HaveOccurred())
+				dReader, err := client.NewDelegatingClient(client.NewDelegatingClientInput{
+					CacheReader:       cachedReader,
+					Client:            cl,
+					CacheUnstructured: true,
+				})
+				Expect(err).NotTo(HaveOccurred())
 
-			actual := &unstructured.UnstructuredList{}
-			actual.SetGroupVersionKind(schema.GroupVersionKind{
-				Group:   "apps",
-				Kind:    "DeploymentList",
-				Version: "v1",
+				actual := &unstructured.UnstructuredList{}
+				actual.SetGroupVersionKind(schema.GroupVersionKind{
+					Group:   "apps",
+					Kind:    "DeploymentList",
+					Version: "v1",
+				})
+				Expect(dReader.List(context.Background(), actual)).To(Succeed())
+				Expect(1).To(Equal(cachedReader.Called))
 			})
-			Expect(dReader.List(context.Background(), actual)).To(Succeed())
-			Expect(0).To(Equal(cachedReader.Called))
 		})
 	})
 })

pkg/client/split.go

@@ -18,19 +18,22 @@ package client
 
 import (
 	"context"
+	"strings"
 
 	"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"
+
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 )
 
 // NewDelegatingClientInput encapsulates the input parameters to create a new delegating client.
 type NewDelegatingClientInput struct {
-	CacheReader     Reader
-	Client          Client
-	UncachedObjects []Object
+	CacheReader       Reader
+	Client            Client
+	UncachedObjects   []Object
+	CacheUnstructured bool
 }
 
 // NewDelegatingClient creates a new delegating client.
@@ -52,10 +55,11 @@ func NewDelegatingClient(in NewDelegatingClientInput) (Client, error) {
 		scheme: in.Client.Scheme(),
 		mapper: in.Client.RESTMapper(),
 		Reader: &delegatingReader{
-			CacheReader:  in.CacheReader,
-			ClientReader: in.Client,
-			scheme:       in.Client.Scheme(),
-			uncachedGVKs: uncachedGVKs,
+			CacheReader:       in.CacheReader,
+			ClientReader:      in.Client,
+			scheme:            in.Client.Scheme(),
+			uncachedGVKs:      uncachedGVKs,
+			cacheUnstructured: in.CacheUnstructured,
 		},
 		Writer:       in.Client,
 		StatusClient: in.Client,
@@ -90,19 +94,30 @@ type delegatingReader struct {
 	CacheReader  Reader
 	ClientReader Reader
 
-	uncachedGVKs map[schema.GroupVersionKind]struct{}
-	scheme       *runtime.Scheme
+	uncachedGVKs      map[schema.GroupVersionKind]struct{}
+	scheme            *runtime.Scheme
+	cacheUnstructured bool
 }
 
 func (d *delegatingReader) shouldBypassCache(obj runtime.Object) (bool, error) {
 	gvk, err := apiutil.GVKForObject(obj, d.scheme)
 	if err != nil {
 		return false, err
 	}
-	_, isUncached := d.uncachedGVKs[gvk]
-	_, isUnstructured := obj.(*unstructured.Unstructured)
-	_, isUnstructuredList := obj.(*unstructured.UnstructuredList)
-	return isUncached || isUnstructured || isUnstructuredList, nil
+	// TODO: this is producing unsafe guesses that don't actually work,
+	// but it matches ~99% of the cases out there.
+	if meta.IsListType(obj) {
+		gvk.Kind = strings.TrimSuffix(gvk.Kind, "List")
+	}
+	if _, isUncached := d.uncachedGVKs[gvk]; isUncached {
+		return true, nil
+	}
+	if !d.cacheUnstructured {
+		_, isUnstructured := obj.(*unstructured.Unstructured)
+		_, isUnstructuredList := obj.(*unstructured.UnstructuredList)
+		return isUnstructured || isUnstructuredList, nil
+	}
+	return false, nil
 }
 
 // Get retrieves an obj for a given object key from the Kubernetes Cluster.

pkg/log/log.go

@@ -35,18 +35,50 @@ package log
 
 import (
 	"context"
+	"sync"
+	"time"
 
 	"github.com/go-logr/logr"
 )
 
 // SetLogger sets a concrete logging implementation for all deferred Loggers.
 func SetLogger(l logr.Logger) {
+	loggerWasSetLock.Lock()
+	defer loggerWasSetLock.Unlock()
+
+	loggerWasSet = true
 	Log.Fulfill(l)
 }
 
+// It is safe to assume that if this wasn't set within the first 30 seconds of a binaries
+// lifetime, it will never get set. The DelegatingLogger causes a high number of memory
+// allocations when not given an actual Logger, so we set a NullLogger to avoid that.
+//
+// We need to keep the DelegatingLogger because we have various inits() that get a logger from
+// here. They will always get executed before any code that imports controller-runtime
+// has a chance to run and hence to set an actual logger.
+func init() {
+	// Init is blocking, so start a new goroutine
+	go func() {
+		time.Sleep(30 * time.Second)
+		loggerWasSetLock.Lock()
+		defer loggerWasSetLock.Unlock()
+		if !loggerWasSet {
+			Log.Fulfill(NullLogger{})
+		}
+	}()
+}
+
+var (
+	loggerWasSetLock sync.Mutex
+	loggerWasSet     bool
+)
+
 // Log is the base logger used by kubebuilder.  It delegates
-// to another logr.Logger.  You *must* call SetLogger to
-// get any actual logging.
+// to another logr.Logger. You *must* call SetLogger to
+// get any actual logging. If SetLogger is not called within
+// the first 30 seconds of a binaries lifetime, it will get
+// set to a NullLogger.
 var Log = NewDelegatingLogger(NullLogger{})
 
 // FromContext returns a logger with predefined values from a context.Context.

pkg/webhook/admission/response.go

@@ -90,8 +90,14 @@ func PatchResponseFromRaw(original, current []byte) Response {
 	return Response{
 		Patches: patches,
 		AdmissionResponse: admissionv1.AdmissionResponse{
-			Allowed:   true,
-			PatchType: func() *admissionv1.PatchType { pt := admissionv1.PatchTypeJSONPatch; return &pt }(),
+			Allowed: true,
+			PatchType: func() *admissionv1.PatchType {
+				if len(patches) == 0 {
+					return nil
+				}
+				pt := admissionv1.PatchTypeJSONPatch
+				return &pt
+			}(),
 		},
 	}
 }