From 8ace87b7ad51aedc767d2e570720258b8514f81a Mon Sep 17 00:00:00 2001 From: Yiqi Gao Date: Mon, 26 Apr 2021 21:00:42 -0400 Subject: [PATCH] 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. --- incubator/hnc/internal/forest/namespace.go | 9 +- .../internal/reconcilers/hierarchy_config.go | 24 +++-- .../hnc/internal/reconcilers/object_test.go | 100 ++++++++++++++++++ .../internal/reconcilers/test_helpers_test.go | 16 +++ 4 files changed, 138 insertions(+), 11 deletions(-) diff --git a/incubator/hnc/internal/forest/namespace.go b/incubator/hnc/internal/forest/namespace.go index 4d53d94f5..3dd5f3cd3 100644 --- a/incubator/hnc/internal/forest/namespace.go +++ b/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. diff --git a/incubator/hnc/internal/reconcilers/hierarchy_config.go b/incubator/hnc/internal/reconcilers/hierarchy_config.go index 05bf4564f..a9b93f681 100644 --- a/incubator/hnc/internal/reconcilers/hierarchy_config.go +++ b/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) { diff --git a/incubator/hnc/internal/reconcilers/object_test.go b/incubator/hnc/internal/reconcilers/object_test.go index 4bf784b7b..5245cb2a8 100644 --- a/incubator/hnc/internal/reconcilers/object_test.go +++ b/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() { diff --git a/incubator/hnc/internal/reconcilers/test_helpers_test.go b/incubator/hnc/internal/reconcilers/test_helpers_test.go index 3e08afadc..1d39a3524 100644 --- a/incubator/hnc/internal/reconcilers/test_helpers_test.go +++ b/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()) }