Support 'ignore' mode in ConfigReconciler

This PR adds the support for 'ignore' mode in ConfigReconciler. Specifically, it adds following features:

If the mode of a type is set to 'ignore', the corresponding object reconciler will ignore all subsequent reconciliations (e.g., new or changed objects will not be propagated).
If the mode of a type is changed from 'ignore' to 'propagate', reconciliations for all existing objects of the type in the cluster will be triggered.
If a mode of a type is unrecognized, it will be treated as 'ignore'.
If a mode of a type is unset, it will be treated as 'propagate'.
Tested: GKE cluster; unit tests.

This PR includes changes introduced in #462

Design doc: http://bit.ly/hnc-type-configuration
Issue: #411

incubator/hnc/api/v1alpha1/hnc_config.go

@@ -55,7 +55,8 @@ type TypeSynchronizationSpec struct {
 	APIVersion string `json:"apiVersion,omitempty"`
 	// Kind to be configured.
 	Kind string `json:"kind,omitempty"`
-	// Synchronization mode of the kind.
+	// Synchronization mode of the kind. If the field is empty, it will be treated
+	// as "propagate". An unsupported mode will be treated as "ignore".
 	// +optional
 	Mode SynchronizationMode `json:"mode,omitempty"`
 }

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

@@ -50,7 +50,9 @@ spec:
                     description: Kind to be configured.
                     type: string
                   mode:
-                    description: Synchronization mode of the kind.
+                    description: Synchronization mode of the kind. If the field is
+                      empty, it will be treated as "propagate". An unsupported mode
+                      will be treated as "ignore".
                     type: string
                 type: object
               type: array

incubator/hnc/config/samples/hnc_v1_config.yaml

@@ -4,9 +4,9 @@ metadata:
   name: config
 spec:
   types:
-    - apiVersion: v1
-      kind: Secret
-      mode: ignore
+    - apiVersion: stable.example.com/v1
+      kind: CronTab
+      mode: remove
     - apiVersion: rbac.authorization.k8s.io/v1
       kind: Role
       mode: propagate

incubator/hnc/go.sum

@@ -175,14 +175,17 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v0.9.0 h1:tXuTFVHC03mW0D+Ua1Q2d1EAVqLTuggX50V0VLICCzY=
 github.com/prometheus/client_golang v0.9.0/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw=
+github.com/prometheus/client_golang v0.9.2 h1:awm861/B8OKDd2I/6o1dy3ra4BamzKhYOiGItCeZ740=
 github.com/prometheus/client_golang v0.9.2/go.mod h1:OsXs2jCmiKlQ1lTBmv21f2mNfw4xf/QclQDMrYNZzcM=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910 h1:idejC8f05m9MGOsuEi1ATq9shN03HrxNkD/luQvxCv8=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
 github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e h1:n/3MEhJQjQxrOUCzh1Y3Re6aJUUWRp2M9+Oc3eVn/54=
 github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e/go.mod h1:daVV7qP5qjZbuso7PdcryaAu0sAZbrN9i7WWcTMWvro=
+github.com/prometheus/common v0.0.0-20181126121408-4724e9255275 h1:PnBWHBf+6L0jOqq0gIVUe6Yk0/QMZ640k6NvkxcBf+8=
 github.com/prometheus/common v0.0.0-20181126121408-4724e9255275/go.mod h1:daVV7qP5qjZbuso7PdcryaAu0sAZbrN9i7WWcTMWvro=
 github.com/prometheus/procfs v0.0.0-20180725123919-05ee40e3a273 h1:agujYaXJSxSo18YNX3jzl+4G6Bstwt+kqv47GS12uL0=
 github.com/prometheus/procfs v0.0.0-20180725123919-05ee40e3a273/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
+github.com/prometheus/procfs v0.0.0-20181204211112-1dc9a6cbc91a h1:9a8MnZMP0X2nLJdBg+pBmGgkJlSaKC2KaQmTCk1XDtE=
 github.com/prometheus/procfs v0.0.0-20181204211112-1dc9a6cbc91a/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
 github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g=

incubator/hnc/pkg/forest/forest.go

@@ -29,6 +29,9 @@ type TypeSyncer interface {
 	SyncNamespace(context.Context, logr.Logger, string) error
 	// Provides the GVK that is handled by the reconciler who implements the interface.
 	GetGVK() schema.GroupVersionKind
+	// SetMode sets the propagation mode of objects that are handled by the reconciler who implements the interface.
+	// The method also syncs objects in the cluster for the type handled by the reconciler if necessary.
+	SetMode(context.Context, api.SynchronizationMode, logr.Logger) error
 }
 
 // Forest defines a forest of namespaces - that is, a set of trees. It includes methods to mutate
@@ -40,7 +43,7 @@ type Forest struct {
 	lock       sync.Mutex
 	namespaces namedNamespaces
 
-	// types is a list of other reconcillers that HierarchyReconciler can call if the hierarchy
+	// types is a list of other reconcilers that HierarchyReconciler can call if the hierarchy
 	// changes. This will force all objects to be re-propagated.
 	//
 	// This is probably wildly inefficient, and we can probably make better use of things like
@@ -72,14 +75,15 @@ func (f *Forest) AddTypeSyncer(nss TypeSyncer) {
 	f.types = append(f.types, nss)
 }
 
-// HasTypeSyncer returns true if there is already a reconciler with the given GVK.
-func (f *Forest) HasTypeSyncer(gvk schema.GroupVersionKind) bool {
+// GetTypeSyncer returns the reconciler for the given GVK or nil if the reconciler
+// does not exist.
+func (f *Forest) GetTypeSyncer(gvk schema.GroupVersionKind) TypeSyncer {
 	for _, t := range f.types {
 		if t.GetGVK() == gvk {
-			return true
+			return t
 		}
 	}
-	return false
+	return nil
 }
 
 // GetTypeSyncers returns the types list.
@@ -115,6 +119,15 @@ func (f *Forest) Get(nm string) *Namespace {
 	return ns
 }
 
+// GetNamespaceNames returns names of all namespaces in the cluster.
+func (f *Forest) GetNamespaceNames() []string {
+	var keys []string
+	for k := range f.namespaces {
+		keys = append(keys, k)
+	}
+	return keys
+}
+
 type namedNamespaces map[string]*Namespace
 
 // TODO Store source objects by GK in the forest - https://github.com/kubernetes-sigs/multi-tenancy/issues/281

incubator/hnc/pkg/reconcilers/hnc_config.go

@@ -50,35 +50,35 @@ func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	// Validate the singleton name.
 	// TODO: Add a validating admission controller to prevent the problem in the first place.
 	if err := r.validateSingletonName(ctx, req.NamespacedName.Name); err != nil {
-		r.Log.Error(err, "Singleton name validation failed.")
+		r.Log.Error(err, "Singleton name validation failed")
 		return ctrl.Result{}, nil
 	}
 
 	inst, err := r.getSingleton(ctx)
 	if err != nil {
-		r.Log.Error(err, "Couldn't read singleton.")
+		r.Log.Error(err, "Couldn't read singleton")
 		return ctrl.Result{}, err
 	}
 
 	// TODO: Modify this and other reconcilers (e.g., hierarchy and object reconcilers) to
 	// achieve the reconciliation.
-	r.Log.Info("Reconciling cluster-wide HNC configuration.")
+	r.Log.Info("Reconciling cluster-wide HNC configuration")
 
 	// Clear the existing conditions because we will reconstruct the latest conditions.
 	inst.Status.Conditions = nil
 
-	// Create corresponding ObjectReconcilers for newly added types, if needed.
-	// TODO: Rename the method syncObjectReconcilers because we might need more than creating ObjectReconcilers in future.
-	// For example, we might need to delete an ObjectReconciler if its corresponding type is deleted in the HNCConfiguration.
-	r.createObjectReconcilers(inst)
+	// Create or sync corresponding ObjectReconcilers, if needed.
+	syncErr := r.syncObjectReconcilers(ctx, inst)
 
 	// Write back to the apiserver.
 	// TODO: Update HNCConfiguration.Status before writing the singleton back to the apiserver.
 	if err := r.writeSingleton(ctx, inst); err != nil {
-		r.Log.Error(err, "Couldn't write singleton.")
+		r.Log.Error(err, "Couldn't write singleton")
 		return ctrl.Result{}, err
 	}
-	return ctrl.Result{}, nil
+
+	// Retry reconciliation if there is a sync error.
+	return ctrl.Result{}, syncErr
 }
 
 // getSingleton returns the singleton if it exists, or creates a default one if it doesn't.
@@ -120,11 +120,20 @@ func (r *ConfigReconciler) writeSingleton(ctx context.Context, inst *api.HNCConf
 	return nil
 }
 
-// createObjectReconcilers creates corresponding ObjectReconcilers for the newly added types in the
-// HNC configuration, if there is any. It also informs the in-memory forest about the newly created
-// ObjectReconcilers. If a type is misconfigured, the corresponding object reconciler will not be created.
-func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) {
-	// This method is guarded by the forest mutex. The mutex is guarding two actions: creating ObjectReconcilers for
+// syncObjectReconcilers creates or syncs ObjectReconcilers.
+//
+// For newly added types in the HNC configuration, the method will create corresponding ObjectReconcilers and
+// informs the in-memory forest about the newly created ObjectReconcilers. If a newly added type is misconfigured,
+// the corresponding object reconciler will not be created. The method will not return error while creating
+// ObjectReconcilers. Instead, any error will be written into the Status field of the singleton. This is
+// intended to avoid infinite reconciliation when the type is misconfigured.
+//
+// If a type exists, the method will sync the mode of the existing object reconciler
+// and update corresponding objects if needed. An error will be return to trigger reconciliation if sync fails.
+func (r *ConfigReconciler) syncObjectReconcilers(ctx context.Context, inst *api.HNCConfiguration) error {
+	// This method is guarded by the forest mutex.
+	//
+	// For creating an object reconciler, the mutex is guarding two actions: creating ObjectReconcilers for
 	// the newly added types and adding the created ObjectReconcilers to the types list in the Forest (r.Forest.types).
 	//
 	// We use mutex to guard write access to the types list because the list is a shared resource between various
@@ -136,28 +145,36 @@ func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) {
 	// from the forest for the object reconciliation, after we create ObjectReconcilers but before we write the
 	// ObjectReconcilers to the types list. As a result, objects of the newly added types might not be propagated correctly
 	// using the latest forest structure.
+	//
+	// For syncing an object reconciler, the mutex is guarding the read access to the `namespaces` field in the forest. The
+	// `namespaces` field is a shared resource between various reconcilers.
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
 
 	for _, t := range inst.Spec.Types {
 		gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind)
-		if r.Forest.HasTypeSyncer(gvk) {
-			continue
+		if ts := r.Forest.GetTypeSyncer(gvk); ts != nil {
+			if err := ts.SetMode(ctx, t.Mode, r.Log); err != nil {
+				return err // retry the reconciliation
+			}
+		} else {
+			r.createObjectReconciler(gvk, t.Mode, inst)
 		}
-		r.createObjectReconciler(gvk, inst)
 	}
+
+	return nil
 }
 
 // createObjectReconciler creates an ObjectReconciler for the given GVK and informs forest about the reconciler.
-// TODO: May need to pass in spec instead to provide information about Mode to the ObjectReconciler in future.
-func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind, inst *api.HNCConfiguration) {
-	r.Log.Info("Creating an object reconciler.", "GVK", gvk)
+func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind, mode api.SynchronizationMode, inst *api.HNCConfiguration) {
+	r.Log.Info("Creating an object reconciler", "gvk", gvk, "mode", mode)
 
 	or := &ObjectReconciler{
 		Client:            r.Client,
 		Log:               ctrl.Log.WithName("reconcilers").WithName(gvk.Kind),
 		Forest:            r.Forest,
 		GVK:               gvk,
+		Mode:              mode,
 		Affected:          make(chan event.GenericEvent),
 		AffectedNamespace: r.HierarchyConfigUpdates,
 	}
@@ -188,7 +205,7 @@ func (r *ConfigReconciler) validateSingletonName(ctx context.Context, nm string)
 		return err
 	}
 
-	msg := "Singleton name is wrong. It should be 'config'."
+	msg := "Singleton name is wrong. It should be 'config'"
 	condition := api.HNCConfigurationCondition{
 		Code: api.CritSingletonNameInvalid,
 		Msg:  msg,
@@ -220,10 +237,11 @@ func (r *ConfigReconciler) forceInitialReconcile(log logr.Logger, reason string)
 
 // SetupWithManager builds a controller with the reconciler.
 func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	if err := ctrl.NewControllerManagedBy(mgr).
+	err := ctrl.NewControllerManagedBy(mgr).
 		For(&api.HNCConfiguration{}).
 		Watches(&source.Channel{Source: r.Igniter}, &handler.EnqueueRequestForObject{}).
-		Complete(r); err != nil {
+		Complete(r)
+	if err != nil {
 		return err
 	}
 	// Create a default singleton if there is no singleton in the cluster.
@@ -235,6 +253,6 @@ func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	// cache is populated at the reconciliation stage. A default singleton will be
 	// created during the reconciliation if there is no singleton in the cluster.
 	r.forceInitialReconcile(r.Log, "Enforce reconciliation to create a default"+
-		"HNCConfiguration singleton if it does not exist.")
+		"HNCConfiguration singleton if it does not exist")
 	return nil
 }

incubator/hnc/pkg/reconcilers/hnc_config_test.go

@@ -3,6 +3,7 @@ package reconcilers_test
 import (
 	"context"
 	"strings"
+	"time"
 
 	api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1"
 	. "github.com/onsi/ginkgo"
@@ -14,6 +15,11 @@ import (
 )
 
 var _ = Describe("HNCConfiguration", func() {
+	// sleepTime is the time to sleep for objects propagation to take effect.
+	// From experiment it takes ~0.015s for HNC to propagate an object. Setting
+	// the sleep time to 1s should be long enough.
+	// We may need to increase the sleep time in future if HNC takes longer to propagate objects.
+	const sleepTime = 1 * time.Second
 	ctx := context.Background()
 
 	var (
@@ -88,22 +94,66 @@ var _ = Describe("HNCConfiguration", func() {
 		Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeFalse())
 	})
 
-	It("should only propagate objects whose types are in HNCConfiguration", func() {
+	It("should not propagate objects if the type is not in HNCConfiguration", func() {
 		setParent(ctx, barName, fooName)
-		makeSecret(ctx, fooName, "foo-sec")
 		makeResourceQuota(ctx, fooName, "foo-resource-quota")
 
+		// Foo should have "foo-resource-quota" since we created there.
+		Eventually(hasResourceQuota(ctx, fooName, "foo-resource-quota")).Should(BeTrue())
+		// Sleep to give "foo-resource-quota" a chance to propagate from foo to bar, if it could.
+		time.Sleep(sleepTime)
+		Expect(hasResourceQuota(ctx, barName, "foo-resource-quota")()).Should(BeFalse())
+	})
+
+	It("should propagate objects if the mode of a type is set to propagate", func() {
 		addToHNCConfig(ctx, "v1", "Secret", api.Propagate)
 
-		// Foo should have both "foo-sec" and "foo-resource-quota" since we created there.
+		setParent(ctx, barName, fooName)
+		makeSecret(ctx, fooName, "foo-sec")
+
+		// Foo should have "foo-sec" since we created there.
 		Eventually(hasSecret(ctx, fooName, "foo-sec")).Should(BeTrue())
-		Eventually(hasResourceQuota(ctx, fooName, "foo-resource-quota")).Should(BeTrue())
 		// "foo-sec" should now be propagated from foo to bar.
 		Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeTrue())
 		Expect(secretInheritedFrom(ctx, barName, "foo-sec")).Should(Equal(fooName))
-		// "foo-resource-quota" should not be propagated from foo to bar because ResourceQuota
-		// is not added to HNCConfiguration.
-		Expect(hasResourceQuota(ctx, barName, "foo-resource-quota")).Should(BeFalse())
+	})
+
+	It("should stop propagating objects if the mode of a type is changed from propagate to ignore", func() {
+		addToHNCConfig(ctx, "v1", "Secret", api.Propagate)
+
+		setParent(ctx, barName, fooName)
+		makeSecret(ctx, fooName, "foo-sec")
+
+		// Foo should have "foo-sec" since we created there.
+		Eventually(hasSecret(ctx, fooName, "foo-sec")).Should(BeTrue())
+		// "foo-sec" should now be propagated from foo to bar because we set the mode of Secret to "propagate".
+		Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeTrue())
+		Expect(secretInheritedFrom(ctx, barName, "foo-sec")).Should(Equal(fooName))
+
+		updateHNCConfigSpec(ctx, "v1", "v1", "Secret", "Secret", api.Propagate, api.Ignore)
+		bazName := createNS(ctx, "baz")
+		setParent(ctx, bazName, fooName)
+		// Sleep to give "foo-sec" a chance to propagate from foo to baz, if it could.
+		time.Sleep(sleepTime)
+		Expect(hasSecret(ctx, bazName, "foo-sec")()).Should(BeFalse())
+	})
+
+	It("should propagate objects if the mode of a type is changed from ignore to propagate", func() {
+		addToHNCConfig(ctx, "v1", "ResourceQuota", api.Ignore)
+
+		setParent(ctx, barName, fooName)
+		makeResourceQuota(ctx, fooName, "foo-resource-quota")
+
+		// Foo should have "foo-resource-quota" since we created there.
+		Eventually(hasResourceQuota(ctx, fooName, "foo-resource-quota")).Should(BeTrue())
+		// Sleep to give "foo-resource-quota" a chance to propagate from foo to bar, if it could.
+		time.Sleep(sleepTime)
+		Expect(hasResourceQuota(ctx, barName, "foo-resource-quota")()).Should(BeFalse())
+
+		updateHNCConfigSpec(ctx, "v1", "v1", "ResourceQuota", "ResourceQuota", api.Ignore, api.Propagate)
+		// "foo-resource-quota" should now be propagated from foo to bar because the mode of ResourceQuota is set to "propagate".
+		Eventually(hasResourceQuota(ctx, barName, "foo-resource-quota")).Should(BeTrue())
+		Expect(resourceQuotaInheritedFrom(ctx, barName, "foo-resource-quota")).Should(Equal(fooName))
 	})
 })
 
@@ -202,12 +252,28 @@ func makeResourceQuota(ctx context.Context, nsName, resourceQuotaName string) {
 	ExpectWithOffset(1, k8sClient.Create(ctx, resourceQuota)).Should(Succeed())
 }
 
-func hasResourceQuota(ctx context.Context, nsName, resourceQuotaName string) bool {
+func hasResourceQuota(ctx context.Context, nsName, resourceQuotaName string) func() bool {
 	// `Eventually` only works with a fn that doesn't take any args
+	return func() bool {
+		nnm := types.NamespacedName{Namespace: nsName, Name: resourceQuotaName}
+		resourceQuota := &corev1.ResourceQuota{}
+		err := k8sClient.Get(ctx, nnm, resourceQuota)
+		return err == nil
+	}
+}
+
+func resourceQuotaInheritedFrom(ctx context.Context, nsName, resourceQuotaName string) string {
 	nnm := types.NamespacedName{Namespace: nsName, Name: resourceQuotaName}
 	resourceQuota := &corev1.ResourceQuota{}
-	err := k8sClient.Get(ctx, nnm, resourceQuota)
-	return err == nil
+	if err := k8sClient.Get(ctx, nnm, resourceQuota); err != nil {
+		// should have been caught above
+		return err.Error()
+	}
+	if resourceQuota.ObjectMeta.Labels == nil {
+		return ""
+	}
+	lif, _ := resourceQuota.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"]
+	return lif
 }
 
 func hasHNCConfigurationConditionWithMsg(ctx context.Context, code api.HNCConfigurationCode, subMsg string) func() bool {