Inclusive configuration of resources to propagate

See issue kubernetes-retired#16. To allow inclusive propagation of resources an
additional SyncornizationMode called 'Allow' which only enables
propagation when a selector is set is added. An 'all' selector is also addded.

Tested: e2e-testing covering secrets resource in 'Allow' mode and checking
propagation when selectors are set and unset ('select', 'treeSelect', 'none', 'all').
Unit testing is also modified to account for the new 'all' selection

Signed-off-by: mzeevi <[email protected]>

Author: root

api/v1alpha2/hierarchy_types.go

@@ -37,6 +37,7 @@ const (
 	AnnotationSelector     = AnnotationPropagatePrefix + "/select"
 	AnnotationTreeSelector = AnnotationPropagatePrefix + "/treeSelect"
 	AnnotationNoneSelector = AnnotationPropagatePrefix + "/none"
+	AnnotationAllSelector  = AnnotationPropagatePrefix + "/all"
 
 	// LabelManagedByStandard will eventually replace our own managed-by annotation (we didn't know
 	// about this standard label when we invented our own).

api/v1alpha2/hnc_config.go

@@ -46,6 +46,10 @@ const (
 
 	// Remove all existing propagated copies.
 	Remove SynchronizationMode = "Remove"
+
+	// Allow allows propagation of objects from ancestors to descendants
+	// and deletes obsolete descendants only if a an annotation is set on the object
+	Allow SynchronizationMode = "Allow"
 )
 
 const (
@@ -94,7 +98,7 @@ type ResourceSpec struct {
 	// Synchronization mode of the kind. If the field is empty, it will be treated
 	// as "Propagate".
 	// +optional
-	// +kubebuilder:validation:Enum=Propagate;Ignore;Remove
+	// +kubebuilder:validation:Enum=Propagate;Ignore;Remove;Allow
 	Mode SynchronizationMode `json:"mode,omitempty"`
 }
 

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)

config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml

@@ -64,6 +64,7 @@ spec:
                       - Propagate
                       - Ignore
                       - Remove
+                      - Allow
                       type: string
                     resource:
                       description: Resource to be configured.

docs/releasing.md

@@ -1,7 +1,7 @@
 # Releasing HNC
 
 HNC uses semantic versioning. We use long-lived branches for every minor
-release, and each release is tagged in Git. Usually, a release will be preceeded
+release, and each release is tagged in Git. Usually, a release will be preceded
 by one or more release candidates. Therefore, at a high level, the flow to
 release a new _minor_ version of HNC is:
 

internal/anchor/reconciler.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -51,9 +52,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
@@ -81,9 +79,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	// Anchors in unmanaged namespace should be ignored. Make sure it
 	// doesn't have any finalizers, otherwise, leave it alone.
 	if why := config.WhyUnmanaged(pnm); why != "" {
-		if len(inst.ObjectMeta.Finalizers) > 0 {
+		if controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Removing finalizers from anchor in unmanaged namespace", "reason", why)
-			inst.ObjectMeta.Finalizers = nil
+			controllerutil.RemoveFinalizer(inst, api.MetaGroup)
 			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 		}
 		return ctrl.Result{}, nil
@@ -94,10 +92,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	// been bypassed and the anchor has been successfully created. Forbidden
 	// anchors won't have finalizers.
 	if why := config.WhyUnmanaged(nm); why != "" {
-		if inst.Status.State != api.Forbidden || len(inst.ObjectMeta.Finalizers) > 0 {
+		if inst.Status.State != api.Forbidden || controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Setting forbidden state on anchor with unmanaged name", "reason", why)
 			inst.Status.State = api.Forbidden
-			inst.ObjectMeta.Finalizers = nil
+			controllerutil.RemoveFinalizer(inst, api.MetaGroup)
 			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 		}
 		return ctrl.Result{}, nil
@@ -113,9 +111,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	r.updateState(log, inst, snsInst)
 
 	// Handle the case where the anchor is being deleted.
-	if deleting, err := r.onDeleting(ctx, log, inst, snsInst); deleting {
-		return ctrl.Result{}, err
+	if !inst.DeletionTimestamp.IsZero() {
+		// Stop reconciliation as anchor is being deleted
+		return ctrl.Result{}, r.onDeleting(ctx, log, inst, snsInst)
 	}
+	// Add finalizers on all non-forbidden anchors to ensure it's not deleted until
+	// after the subnamespace is deleted.
+	controllerutil.AddFinalizer(inst, api.MetaGroup)
 
 	// If the subnamespace doesn't exist, create it.
 	if inst.Status.State == api.Missing {
@@ -130,9 +132,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		}
 	}
 
-	// Add finalizers on all non-forbidden anchors to ensure it's not deleted until
-	// after the subnamespace is deleted.
-	inst.ObjectMeta.Finalizers = []string{api.MetaGroup}
 	return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 }
 
@@ -145,19 +144,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 // There are several conditions where we skip step 1 - for example, if we're uninstalling HNC, or
 // if allowCascadingDeletion is disabled but the subnamespace has descendants (see
 // shouldDeleteSubns for details). In such cases, we move straight to step 2.
-func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) (bool, error) {
-	// Early exit and continue reconciliation if the instance is not being deleted.
-	if inst.DeletionTimestamp.IsZero() {
-		return false, nil
-	}
-
+func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) error {
 	// We handle deletions differently depending on whether _one_ anchor is being deleted (i.e., the
 	// user wants to delete the namespace) or whether the Anchor CRD is being deleted, which usually
 	// means HNC is being uninstalled and we shouldn't delete _any_ namespaces.
 	deletingCRD, err := crd.IsDeletingCRD(ctx, api.Anchors)
 	if err != nil {
 		log.Error(err, "Couldn't determine if CRD is being deleted")
-		return false, err
+		return err
 	}
 	log.V(1).Info("Anchor is being deleted", "deletingCRD", deletingCRD)
 
@@ -166,21 +160,21 @@ func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.
 	switch {
 	case r.shouldDeleteSubns(log, inst, snsInst, deletingCRD):
 		log.Info("Deleting subnamespace due to anchor being deleted")
-		return true, r.deleteNamespace(ctx, log, snsInst)
+		return r.deleteNamespace(ctx, log, snsInst)
 	case r.shouldFinalizeAnchor(log, inst, snsInst):
 		log.V(1).Info("Unblocking deletion") // V(1) since we'll very shortly show an "anchor deleted" message
-		inst.ObjectMeta.Finalizers = nil
-		return true, r.writeInstance(ctx, log, inst)
+		controllerutil.RemoveFinalizer(inst, api.MetaGroup)
+		return r.writeInstance(ctx, log, inst)
 	default:
 		// There's nothing to do; we're just waiting for something to happen. Print out a log message
 		// indicating what we're waiting for.
-		if len(inst.ObjectMeta.Finalizers) > 0 {
+		if controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Waiting for subnamespace to be fully purged before letting the anchor be deleted")
 		} else {
 			// I doubt we'll ever get here but I suppose it's possible
 			log.Info("Waiting for K8s to delete this anchor (all finalizers are removed)")
 		}
-		return true, nil
+		return nil
 	}
 }
 
@@ -213,7 +207,7 @@ func (r *Reconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAn
 			log.V(1).Info("The subnamespace is already being deleted; no need to delete again")
 			return false
 		}
-		if len(inst.ObjectMeta.Finalizers) == 0 {
+		if !controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.V(1).Info("The anchor has already been finalized; do not reconsider deleting the namespace")
 			return false
 		}
@@ -257,7 +251,7 @@ func (r *Reconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAn
 // deleted, it's in the process of being deleted, etc).
 func (r *Reconciler) shouldFinalizeAnchor(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) bool {
 	// If the anchor is already finalized, there's no need to do it again.
-	if len(inst.ObjectMeta.Finalizers) == 0 {
+	if !controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 		return false
 	}
 
@@ -348,10 +342,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 +360,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 +382,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 +399,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/anchor/validator.go

@@ -97,13 +97,11 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 		// Can't create subnamespaces in unmanaged namespaces
 		if why := config.WhyUnmanaged(pnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace in the unmanaged namespace %q (%s)", pnm, why)
-			// TODO(erikgb): Add to list of Invalid field errors?
 			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, pnm, err)
 		}
 		// Can't create subnamespaces using unmanaged namespace names
 		if why := config.WhyUnmanaged(cnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace using the unmanaged namespace name %q (%s)", cnm, why)
-			// TODO(erikgb): Add to list of Invalid field errors?
 			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, cnm, err)
 		}
 
@@ -113,7 +111,6 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 			childIsMissingAnchor := (cns.Parent().Name() == pnm && cns.IsSub)
 			if !childIsMissingAnchor {
 				err := errors.New("cannot create a subnamespace using an existing namespace")
-				// TODO(erikgb): Add to list of Invalid field errors?
 				return webhooks.DenyConflict(api.SubnamespaceAnchorGR, cnm, err)
 			}
 		}

internal/config/cache_unstructured.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
+}