Sync object propagation on newly labeled namespaces

Fix 1448

Detect namespace label changes. If the labels are updated, ensure
syncing all the objects (including non-existing yet but possible
propagated objects) in the namespace, so that we will not miss any
propagation label changes that should trigger propagation or
unpropagation.

Add integration tests in reconciler/object_test.go.

Tested manually that labeling and unlabeling a child with the propagate
label won't do anything before the fix, but triggers propagation and
unpropagation accordingly after the fix. Also tested with `make test`,
passed after the fix ant failed before the fix.

Author: yiqigao217

incubator/hnc/internal/forest/namespace.go

@@ -1,6 +1,8 @@
 package forest
 
 import (
+	"reflect"
+
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -92,12 +94,15 @@ func (ns *Namespace) GetLabels() labels.Set {
 	return labels.Set(ns.labels)
 }
 
-// Deep copy the input labels so that it'll not be changed after
-func (ns *Namespace) SetLabels(labels map[string]string) {
+// Deep copy the input labels so that it'll not be changed after. It returns
+// true if the labels are updated; returns false if there's no change.
+func (ns *Namespace) SetLabels(labels map[string]string) bool {
+	updated := !reflect.DeepEqual(ns.labels, labels)
 	ns.labels = make(map[string]string)
 	for key, val := range labels {
 		ns.labels[key] = val
 	}
+	return updated
 }
 
 // clean garbage collects this namespace if it has a zero value.

incubator/hnc/internal/reconcilers/hierarchy_config.go

@@ -144,13 +144,13 @@ func (r *HierarchyConfigReconciler) reconcile(ctx context.Context, log logr.Logg
 	r.updateFinalizers(ctx, log, inst, nsInst, anms)
 
 	// Sync the Hierarchy singleton with the in-memory forest.
-	initial := r.syncWithForest(log, nsInst, inst, deletingCRD, anms)
+	needUpdateObjects := r.syncWithForest(log, nsInst, inst, deletingCRD, anms)
 
 	// Write back if anything's changed. Early-exit if we just write back exactly what we had and this
 	// isn't the first time we're syncing.
 	updated, err := r.writeInstances(ctx, log, origHC, inst, origNS, nsInst)
-	updated = updated || initial
-	if !updated || err != nil {
+	needUpdateObjects = updated || needUpdateObjects
+	if !needUpdateObjects || err != nil {
 		return err
 	}
 
@@ -223,7 +223,8 @@ func (r *HierarchyConfigReconciler) updateFinalizers(ctx context.Context, log lo
 // guarded by the forest mutex, which means that none of the other namespaces being reconciled will
 // be able to proceed until this one is finished. While the results of the reconiliation may not be
 // fully written back to the apiserver yet, each namespace is reconciled in isolation (apart from
-// the in-memory forest) so this is fine.
+// the in-memory forest) so this is fine. Return true, if the namespace is just synced or the
+// namespace labels are changed that requires updating all objects in the namespaces.
 func (r *HierarchyConfigReconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, inst *api.HierarchyConfiguration, deletingCRD bool, anms []string) bool {
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
@@ -257,9 +258,9 @@ func (r *HierarchyConfigReconciler) syncWithForest(log logr.Logger, nsInst *core
 	r.syncConditions(log, inst, ns, deletingCRD, hadCrit)
 
 	// Sync the tree labels. This should go last since it can depend on the conditions.
-	r.syncLabel(log, nsInst, ns)
+	nsCustomerLabelUpdated := r.syncLabel(log, nsInst, ns)
 
-	return initial
+	return initial || nsCustomerLabelUpdated
 }
 
 // syncExternalNamespace sets external tree labels to the namespace in the forest
@@ -420,10 +421,11 @@ func (r *HierarchyConfigReconciler) syncAnchors(log logr.Logger, ns *forest.Name
 	}
 }
 
-func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) {
+// Sync namespace tree labels and other labels. Return true if the labels are updated.
+func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool {
 	if ns.IsExternal() {
 		metadata.SetLabel(nsInst, nsInst.Name+api.LabelTreeDepthSuffix, "0")
-		return
+		return false
 	}
 
 	// Remove all existing depth labels.
@@ -461,7 +463,11 @@ func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Na
 	}
 	// Update the labels in the forest so that we can quickly access the labels and
 	// compare if they match the given selector
-	ns.SetLabels(nsInst.Labels)
+	if ns.SetLabels(nsInst.Labels) {
+		log.Info("Namespace labels have been updated.")
+		return true
+	}
+	return false
 }
 
 func (r *HierarchyConfigReconciler) syncConditions(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace, deletingCRD, hadCrit bool) {

incubator/hnc/internal/reconcilers/object_test.go

@@ -231,6 +231,106 @@ var _ = Describe("Exceptions", func() {
 			})
 		}
 	})
+
+	Context("Update the descendant namespaces after 'select' exception annotation is set", func() {
+		const (
+			label        = "propagate-label"
+			p            = "parent"
+			labeledchild = "labeledchild"
+			nolabelchild = "nolabelchild"
+			labeledns    = "labeledns"
+			nolabelns    = "nolabelns"
+		)
+		tests := []struct {
+			name       string
+			toLabel    string
+			toUnlabel  string
+			toAddChild string
+			want       []string
+			notWant    []string
+		}{{
+			name:    "propagate object only to children with the label",
+			want:    []string{labeledchild},
+			notWant: []string{nolabelchild},
+		}, {
+			name:    "propagate object to a newly-labeled child - issue #1448",
+			toLabel: nolabelchild,
+			want:    []string{labeledchild, nolabelchild},
+			notWant: []string{},
+		}, {
+			name:      "not propagate object to a newly-unlabeled child - issue #1448",
+			toUnlabel: labeledchild,
+			want:      []string{},
+			notWant:   []string{labeledchild, nolabelchild},
+		}, {
+			name:       "propagate object to a new child with the label",
+			toAddChild: labeledns,
+			want:       []string{labeledchild, labeledns},
+			notWant:    []string{nolabelchild},
+		}, {
+			name:       "not propagate object to a new child without the label",
+			toAddChild: nolabelns,
+			want:       []string{labeledchild},
+			notWant:    []string{nolabelchild, nolabelns},
+		}}
+
+		for _, tc := range tests {
+			// Making a local copy of tc is necessary to ensure the correct value is passed to the closure,
+			// for more details look at https://onsi.github.io/ginkgo/ and search for 'closure'
+			tc := tc
+			It("Should "+tc.name, func() {
+				// Set up namespaces
+				names := map[string]string{
+					p:            createNS(ctx, p),
+					labeledchild: createNSWithLabel(ctx, labeledchild, map[string]string{label: "true"}),
+					nolabelchild: createNS(ctx, nolabelchild),
+					labeledns:    createNSWithLabel(ctx, labeledns, map[string]string{label: "true"}),
+					nolabelns:    createNS(ctx, nolabelns),
+				}
+				setParent(ctx, names[labeledchild], names[p])
+				setParent(ctx, names[nolabelchild], names[p])
+
+				// Create a Role and verify it's propagated to all children.
+				makeObject(ctx, api.RoleResource, names[p], "testrole")
+				Eventually(hasObject(ctx, api.RoleResource, names[labeledchild], "testrole")).Should(BeTrue(), "When propagating testrole to %s", names[labeledchild])
+				Eventually(hasObject(ctx, api.RoleResource, names[nolabelchild], "testrole")).Should(BeTrue(), "When propagating testrole to %s", names[nolabelchild])
+				// Add `select` exception annotation with propagate label and verify the
+				// object is only propagated to children with the label.
+				updateObjectWithAnnotation(ctx, api.RoleResource, names[p], "testrole", map[string]string{
+					api.AnnotationSelector: label,
+				})
+				Eventually(hasObject(ctx, api.RoleResource, names[nolabelchild], "testrole")).Should(BeFalse(), "When propagating testrole to %s", names[nolabelchild])
+				Consistently(hasObject(ctx, api.RoleResource, names[nolabelchild], "testrole")).Should(BeFalse(), "When propagating testrole to %s", names[nolabelchild])
+				Consistently(hasObject(ctx, api.RoleResource, names[labeledchild], "testrole")).Should(BeTrue(), "When propagating testrole to %s", names[labeledchild])
+
+				// Add the label to the namespace if the value is not empty.
+				if tc.toLabel != "" {
+					addNamespaceLabel(ctx, names[tc.toLabel], label, "true")
+				}
+
+				// Unlabel the namespace if the value is not empty.
+				if tc.toUnlabel != "" {
+					removeNamespaceLabel(ctx, names[tc.toUnlabel], label)
+				}
+
+				// Set a new child if the value is not empty.
+				if tc.toAddChild != "" {
+					setParent(ctx, names[tc.toAddChild], names[p])
+				}
+
+				// then check that the objects are kept in these namespaces
+				for _, ns := range tc.want {
+					ns = replaceStrings(ns, names)
+					Eventually(hasObject(ctx, api.RoleResource, ns, "testrole")).Should(BeTrue(), "When propagating testrole to %s", ns)
+				}
+				// make sure the changes are propagated
+				for _, ns := range tc.notWant {
+					ns = replaceStrings(ns, names)
+					Eventually(hasObject(ctx, api.RoleResource, ns, "testrole")).Should(BeFalse(), "When propagating testrole to %s", ns)
+				}
+			})
+		}
+	})
 })
 
 var _ = Describe("Basic propagation", func() {

incubator/hnc/internal/reconcilers/test_helpers_test.go

@@ -108,6 +108,22 @@ func createNSes(ctx context.Context, num int) []string {
 	return nms
 }
 
+func addNamespaceLabel(ctx context.Context, nm, k, v string) {
+	ns := getNamespace(ctx, nm)
+	l := ns.Labels
+	l[k] = v
+	ns.SetLabels(l)
+	updateNamespace(ctx, ns)
+}
+
+func removeNamespaceLabel(ctx context.Context, nm, k string) {
+	ns := getNamespace(ctx, nm)
+	l := ns.Labels
+	delete(l, k)
+	ns.SetLabels(l)
+	updateNamespace(ctx, ns)
+}
+
 func updateNamespace(ctx context.Context, ns *corev1.Namespace) {
 	ExpectWithOffset(1, k8sClient.Update(ctx, ns)).Should(Succeed())
 }