Skip to content

Commit 683a23b

Browse files
committed
Read-only client in webhooks only mode
This appears like a more robust implementation of the webhooks only mode than the current one. It ensures that the client provided to all components are not able to perform any write operations towards the API. Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully. The integration tests were verified both in default and HA mode.
1 parent fdf83ab commit 683a23b

File tree

8 files changed

+110
-74
lines changed

8 files changed

+110
-74
lines changed

cmd/manager/main.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ func createManager() ctrl.Manager {
242242
// it turns out to be harmful.
243243
cfg.Burst = int(cfg.QPS * 1.5)
244244
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
245-
NewClient: config.NewCachingClient,
245+
NewClient: config.NewClient(webhooksOnly),
246246
Scheme: scheme,
247247
MetricsBindAddress: metricsAddr,
248248
HealthProbeBindAddress: probeAddr,
@@ -304,7 +304,6 @@ func startControllers(mgr ctrl.Manager, certsReady chan struct{}) {
304304
opts := setup.Options{
305305
NoWebhooks: noWebhooks,
306306
MaxReconciles: maxReconciles,
307-
ReadOnly: webhooksOnly,
308307
HRQ: enableHRQ,
309308
}
310309
setup.Create(setupLog, mgr, f, opts)

internal/anchor/reconciler.go

-19
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,6 @@ type Reconciler struct {
5151
// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to
5252
// enqueue additional objects that need updating.
5353
affected chan event.GenericEvent
54-
55-
// ReadOnly disables writebacks
56-
ReadOnly bool
5754
}
5855

5956
// 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
348345
}
349346

350347
func (r *Reconciler) writeInstance(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor) error {
351-
if r.ReadOnly {
352-
return nil
353-
}
354-
355348
if inst.CreationTimestamp.IsZero() {
356349
if err := r.Create(ctx, inst); err != nil {
357350
log.Error(err, "while creating on apiserver")
@@ -370,10 +363,6 @@ func (r *Reconciler) writeInstance(ctx context.Context, log logr.Logger, inst *a
370363
// finalizers on the instance before calling this function.
371364
//lint:ignore U1000 Ignore for now, as it may be used again in the future
372365
func (r *Reconciler) deleteInstance(ctx context.Context, inst *api.SubnamespaceAnchor) error {
373-
if r.ReadOnly {
374-
return nil
375-
}
376-
377366
if err := r.Delete(ctx, inst); err != nil {
378367
return fmt.Errorf("while deleting on apiserver: %w", err)
379368
}
@@ -396,10 +385,6 @@ func (r *Reconciler) getNamespace(ctx context.Context, nm string) (*corev1.Names
396385
}
397386

398387
func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pnm string) error {
399-
if r.ReadOnly {
400-
return nil
401-
}
402-
403388
inst := &corev1.Namespace{}
404389
inst.ObjectMeta.Name = nm
405390
metadata.SetAnnotation(inst, api.SubnamespaceOf, pnm)
@@ -417,10 +402,6 @@ func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pn
417402
}
418403

419404
func (r *Reconciler) deleteNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
420-
if r.ReadOnly {
421-
return nil
422-
}
423-
424405
if err := r.Delete(ctx, inst); err != nil {
425406
log.Error(err, "While deleting subnamespace")
426407
return err

internal/config/cache_unstructured.go

-28
This file was deleted.

internal/config/client.go

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package config
2+
3+
import (
4+
"context"
5+
6+
authzv1 "k8s.io/api/authorization/v1"
7+
"k8s.io/apimachinery/pkg/api/meta"
8+
"k8s.io/apimachinery/pkg/runtime"
9+
"k8s.io/client-go/rest"
10+
"sigs.k8s.io/controller-runtime/pkg/cache"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
12+
"sigs.k8s.io/controller-runtime/pkg/cluster"
13+
)
14+
15+
// newCachingClient is an alternative implementation of controller-runtime's
16+
// default client for manager.Manager.
17+
// The only difference is that this implementation sets `CacheUnstructured` to `true` to
18+
// cache unstructured objects.
19+
func newCachingClient(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
20+
c, err := client.New(config, options)
21+
if err != nil {
22+
return nil, err
23+
}
24+
25+
return client.NewDelegatingClient(client.NewDelegatingClientInput{
26+
CacheReader: cache,
27+
Client: c,
28+
UncachedObjects: uncachedObjects,
29+
CacheUnstructured: true,
30+
})
31+
}
32+
33+
func NewClient(readOnly bool) cluster.NewClientFunc {
34+
return func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
35+
c, err := newCachingClient(cache, config, options, uncachedObjects...)
36+
if readOnly {
37+
c = &readOnlyClient{client: c}
38+
}
39+
return c, err
40+
}
41+
}
42+
43+
// readOnlyClient is a client decorator that ensures no write operations are possible.
44+
type readOnlyClient struct {
45+
client client.Client
46+
}
47+
48+
func (c readOnlyClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
49+
return c.client.Get(ctx, key, obj)
50+
}
51+
52+
func (c readOnlyClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
53+
return c.client.List(ctx, list, opts...)
54+
}
55+
56+
func (c readOnlyClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
57+
// Allow callers to create subject access reviews - even in read-only mode.
58+
// Creating a SAR will not modify state in etcd. Used to perform RBAC checks.
59+
if _, ok := obj.(*authzv1.SubjectAccessReview); ok {
60+
return c.client.Create(ctx, obj, opts...)
61+
}
62+
return nil
63+
}
64+
65+
func (c readOnlyClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
66+
return nil
67+
}
68+
69+
func (c readOnlyClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
70+
return nil
71+
}
72+
73+
func (c readOnlyClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
74+
return nil
75+
}
76+
77+
func (c readOnlyClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
78+
return nil
79+
}
80+
81+
func (c readOnlyClient) Status() client.StatusWriter {
82+
return nopStatusWriter{}
83+
}
84+
85+
func (c readOnlyClient) Scheme() *runtime.Scheme {
86+
return c.client.Scheme()
87+
}
88+
89+
func (c readOnlyClient) RESTMapper() meta.RESTMapper {
90+
return c.client.RESTMapper()
91+
}
92+
93+
type nopStatusWriter struct{}
94+
95+
func (w nopStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
96+
return nil
97+
}
98+
99+
func (w nopStatusWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
100+
return nil
101+
}

internal/hierarchyconfig/reconciler.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,6 @@ type Reconciler struct {
7474
// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to
7575
// enqueue additional namespaces that need updating.
7676
affected chan event.GenericEvent
77-
78-
ReadOnly bool
7977
}
8078

8179
type AnchorReconcilerType interface {
@@ -703,7 +701,7 @@ func (r *Reconciler) writeInstances(ctx context.Context, log logr.Logger, newHC
703701
func (r *Reconciler) writeHierarchy(ctx context.Context, log logr.Logger, inst *api.HierarchyConfiguration, isDeletingNS bool) error {
704702
// The inst's name will be blank if it wasn't on the apiserver and we didn't want to make any
705703
// changes to it (e.g. no children, conditions, etc).
706-
if r.ReadOnly || inst.GetName() == "" {
704+
if inst.GetName() == "" {
707705
return nil
708706
}
709707
exists := !inst.CreationTimestamp.IsZero()
@@ -731,10 +729,6 @@ func (r *Reconciler) writeHierarchy(ctx context.Context, log logr.Logger, inst *
731729
}
732730

733731
func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
734-
if r.ReadOnly {
735-
return nil
736-
}
737-
738732
// NB: HCR can't create namespaces, that's only in anchor reconciler
739733
stats.WriteNamespace()
740734
log.V(1).Info("Updating namespace on apiserver")

internal/hncconfig/reconciler.go

-7
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,6 @@ type Reconciler struct {
4848
// that is used to enqueue the singleton to trigger reconciliation.
4949
trigger chan event.GenericEvent
5050

51-
// ReadOnly disables writebacks
52-
ReadOnly bool
53-
5451
// RefreshDuration is the maximum amount of time between refreshes
5552
RefreshDuration time.Duration
5653

@@ -210,10 +207,6 @@ func (r *Reconciler) validateSingleton(inst *api.HNCConfiguration) {
210207
// We will write the singleton to apiserver even it is not changed because we assume this
211208
// reconciler is called very infrequently and is not performance critical.
212209
func (r *Reconciler) writeSingleton(ctx context.Context, inst *api.HNCConfiguration) error {
213-
if r.ReadOnly {
214-
return nil
215-
}
216-
217210
if inst.CreationTimestamp.IsZero() {
218211
// No point creating it if the CRD's being deleted
219212
if isDeleted, err := crd.IsDeletingCRD(ctx, api.HNCConfigSingletons); isDeleted || err != nil {

internal/integtest/setup.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func HNCBeforeSuite() {
9999

100100
By("creating manager")
101101
k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
102-
NewClient: config.NewCachingClient,
102+
NewClient: config.NewClient(false),
103103
MetricsBindAddress: "0", // disable metrics serving since 'go test' runs multiple suites in parallel processes
104104
Scheme: scheme.Scheme,
105105
})

internal/setup/reconcilers.go

+6-10
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818

1919
type Options struct {
2020
MaxReconciles int
21-
ReadOnly bool
2221
UseFakeClient bool
2322
NoWebhooks bool
2423
HNCCfgRefresh time.Duration
@@ -51,10 +50,9 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
5150

5251
// Create Anchor reconciler.
5352
ar := &anchor.Reconciler{
54-
Client: mgr.GetClient(),
55-
Log: ctrl.Log.WithName("anchor").WithName("reconcile"),
56-
Forest: f,
57-
ReadOnly: opts.ReadOnly,
53+
Client: mgr.GetClient(),
54+
Log: ctrl.Log.WithName("anchor").WithName("reconcile"),
55+
Forest: f,
5856
}
5957
f.AddListener(ar)
6058

@@ -64,16 +62,14 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
6462
Log: ctrl.Log.WithName("hncconfig").WithName("reconcile"),
6563
Manager: mgr,
6664
Forest: f,
67-
ReadOnly: opts.ReadOnly,
6865
RefreshDuration: opts.HNCCfgRefresh,
6966
}
7067

7168
// Create the HC reconciler with a pointer to the Anchor reconciler.
7269
hcr := &hierarchyconfig.Reconciler{
73-
Client: mgr.GetClient(),
74-
Log: ctrl.Log.WithName("hierarchyconfig").WithName("reconcile"),
75-
Forest: f,
76-
ReadOnly: opts.ReadOnly,
70+
Client: mgr.GetClient(),
71+
Log: ctrl.Log.WithName("hierarchyconfig").WithName("reconcile"),
72+
Forest: f,
7773
}
7874

7975
if opts.HRQ {

0 commit comments

Comments
 (0)