Read-only client in webhooks only mode

cmd/manager/main.go

@@ -242,7 +242,7 @@ func createManager() ctrl.Manager {
 	// it turns out to be harmful.
 	cfg.Burst = int(cfg.QPS * 1.5)
 	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
-		NewClient:              config.NewCachingClient,
+		NewClient:              config.NewClient(webhooksOnly),
 		Scheme:                 scheme,
 		MetricsBindAddress:     metricsAddr,
 		HealthProbeBindAddress: probeAddr,
@@ -304,7 +304,6 @@ func startControllers(mgr ctrl.Manager, certsReady chan struct{}) {
 	opts := setup.Options{
 		NoWebhooks:    noWebhooks,
 		MaxReconciles: maxReconciles,
-		ReadOnly:      webhooksOnly,
 		HRQ:           enableHRQ,
 	}
 	setup.Create(setupLog, mgr, f, opts)

internal/anchor/reconciler.go

@@ -51,9 +51,6 @@ type Reconciler struct {
 	// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to
 	// enqueue additional objects that need updating.
 	affected chan event.GenericEvent
-
-	// ReadOnly disables writebacks
-	ReadOnly bool
 }
 
 // Reconcile sets up some basic variables and then calls the business logic. It currently
@@ -348,10 +345,6 @@ func (r *Reconciler) getInstance(ctx context.Context, pnm, nm string) (*api.Subn
 }
 
 func (r *Reconciler) writeInstance(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	if inst.CreationTimestamp.IsZero() {
 		if err := r.Create(ctx, inst); err != nil {
 			log.Error(err, "while creating on apiserver")
@@ -370,10 +363,6 @@ func (r *Reconciler) writeInstance(ctx context.Context, log logr.Logger, inst *a
 // finalizers on the instance before calling this function.
 //lint:ignore U1000 Ignore for now, as it may be used again in the future
 func (r *Reconciler) deleteInstance(ctx context.Context, inst *api.SubnamespaceAnchor) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	if err := r.Delete(ctx, inst); err != nil {
 		return fmt.Errorf("while deleting on apiserver: %w", err)
 	}
@@ -396,10 +385,6 @@ func (r *Reconciler) getNamespace(ctx context.Context, nm string) (*corev1.Names
 }
 
 func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pnm string) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	inst := &corev1.Namespace{}
 	inst.ObjectMeta.Name = nm
 	metadata.SetAnnotation(inst, api.SubnamespaceOf, pnm)
@@ -417,10 +402,6 @@ func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pn
 }
 
 func (r *Reconciler) deleteNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	if err := r.Delete(ctx, inst); err != nil {
 		log.Error(err, "While deleting subnamespace")
 		return err

internal/config/client.go

@@ -0,0 +1,101 @@
+package config
+
+import (
+	"context"
+
+	authzv1 "k8s.io/api/authorization/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/cluster"
+)
+
+// newCachingClient is an alternative implementation of controller-runtime's
+// default client for manager.Manager.
+// The only difference is that this implementation sets `CacheUnstructured` to `true` to
+// cache unstructured objects.
+func newCachingClient(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+	c, err := client.New(config, options)
+	if err != nil {
+		return nil, err
+	}
+
+	return client.NewDelegatingClient(client.NewDelegatingClientInput{
+		CacheReader:       cache,
+		Client:            c,
+		UncachedObjects:   uncachedObjects,
+		CacheUnstructured: true,
+	})
+}
+
+func NewClient(readOnly bool) cluster.NewClientFunc {
+	return func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
+		c, err := newCachingClient(cache, config, options, uncachedObjects...)
+		if readOnly {
+			c = &readOnlyClient{client: c}
+		}
+		return c, err
+	}
+}
+
+// readOnlyClient is a client decorator that ensures no write operations are possible.
+type readOnlyClient struct {
+	client client.Client
+}
+
+func (c readOnlyClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
+	return c.client.Get(ctx, key, obj)
+}
+
+func (c readOnlyClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	return c.client.List(ctx, list, opts...)
+}
+
+func (c readOnlyClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+	// Allow callers to create subject access reviews - even in read-only mode.
+	// Creating a SAR will not modify state in etcd. Used to perform RBAC checks.
+	if _, ok := obj.(*authzv1.SubjectAccessReview); ok {
+		return c.client.Create(ctx, obj, opts...)
+	}
+	return nil
+}
+
+func (c readOnlyClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+	return nil
+}
+
+func (c readOnlyClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	return nil
+}
+
+func (c readOnlyClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+	return nil
+}
+
+func (c readOnlyClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
+	return nil
+}
+
+func (c readOnlyClient) Status() client.StatusWriter {
+	return nopStatusWriter{}
+}
+
+func (c readOnlyClient) Scheme() *runtime.Scheme {
+	return c.client.Scheme()
+}
+
+func (c readOnlyClient) RESTMapper() meta.RESTMapper {
+	return c.client.RESTMapper()
+}
+
+type nopStatusWriter struct{}
+
+func (w nopStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	return nil
+}
+
+func (w nopStatusWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+	return nil
+}

internal/hierarchyconfig/reconciler.go

@@ -74,8 +74,6 @@ type Reconciler struct {
 	// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to
 	// enqueue additional namespaces that need updating.
 	affected chan event.GenericEvent
-
-	ReadOnly bool
 }
 
 type AnchorReconcilerType interface {
@@ -703,7 +701,7 @@ func (r *Reconciler) writeInstances(ctx context.Context, log logr.Logger, newHC
 func (r *Reconciler) writeHierarchy(ctx context.Context, log logr.Logger, inst *api.HierarchyConfiguration, isDeletingNS bool) error {
 	// The inst's name will be blank if it wasn't on the apiserver and we didn't want to make any
 	// changes to it (e.g. no children, conditions, etc).
-	if r.ReadOnly || inst.GetName() == "" {
+	if inst.GetName() == "" {
 		return nil
 	}
 	exists := !inst.CreationTimestamp.IsZero()
@@ -731,10 +729,6 @@ func (r *Reconciler) writeHierarchy(ctx context.Context, log logr.Logger, inst *
 }
 
 func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	// NB: HCR can't create namespaces, that's only in anchor reconciler
 	stats.WriteNamespace()
 	log.V(1).Info("Updating namespace on apiserver")

internal/hncconfig/reconciler.go

@@ -48,9 +48,6 @@ type Reconciler struct {
 	// that is used to enqueue the singleton to trigger reconciliation.
 	trigger chan event.GenericEvent
 
-	// ReadOnly disables writebacks
-	ReadOnly bool
-
 	// RefreshDuration is the maximum amount of time between refreshes
 	RefreshDuration time.Duration
 
@@ -210,10 +207,6 @@ func (r *Reconciler) validateSingleton(inst *api.HNCConfiguration) {
 // We will write the singleton to apiserver even it is not changed because we assume this
 // reconciler is called very infrequently and is not performance critical.
 func (r *Reconciler) writeSingleton(ctx context.Context, inst *api.HNCConfiguration) error {
-	if r.ReadOnly {
-		return nil
-	}
-
 	if inst.CreationTimestamp.IsZero() {
 		// No point creating it if the CRD's being deleted
 		if isDeleted, err := crd.IsDeletingCRD(ctx, api.HNCConfigSingletons); isDeleted || err != nil {

internal/integtest/setup.go

@@ -99,7 +99,7 @@ func HNCBeforeSuite() {
 
 	By("creating manager")
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-		NewClient:          config.NewCachingClient,
+		NewClient:          config.NewClient(false),
 		MetricsBindAddress: "0", // disable metrics serving since 'go test' runs multiple suites in parallel processes
 		Scheme:             scheme.Scheme,
 	})

internal/setup/reconcilers.go

@@ -18,7 +18,6 @@ import (
 
 type Options struct {
 	MaxReconciles   int
-	ReadOnly        bool
 	UseFakeClient   bool
 	NoWebhooks      bool
 	HNCCfgRefresh   time.Duration
@@ -51,10 +50,9 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 
 	// Create Anchor reconciler.
 	ar := &anchor.Reconciler{
-		Client:   mgr.GetClient(),
-		Log:      ctrl.Log.WithName("anchor").WithName("reconcile"),
-		Forest:   f,
-		ReadOnly: opts.ReadOnly,
+		Client: mgr.GetClient(),
+		Log:    ctrl.Log.WithName("anchor").WithName("reconcile"),
+		Forest: f,
 	}
 	f.AddListener(ar)
 
@@ -64,16 +62,14 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 		Log:             ctrl.Log.WithName("hncconfig").WithName("reconcile"),
 		Manager:         mgr,
 		Forest:          f,
-		ReadOnly:        opts.ReadOnly,
 		RefreshDuration: opts.HNCCfgRefresh,
 	}
 
 	// Create the HC reconciler with a pointer to the Anchor reconciler.
 	hcr := &hierarchyconfig.Reconciler{
-		Client:   mgr.GetClient(),
-		Log:      ctrl.Log.WithName("hierarchyconfig").WithName("reconcile"),
-		Forest:   f,
-		ReadOnly: opts.ReadOnly,
+		Client: mgr.GetClient(),
+		Log:    ctrl.Log.WithName("hierarchyconfig").WithName("reconcile"),
+		Forest: f,
 	}
 
 	if opts.HRQ {